Change the CONFIG_ECMA_NUMBER_TYPE default value to CONFIG_ECMA_NUMBE… #1229
Conversation
| @@ -171,7 +170,9 @@ elseif(FEATURE_PROFILE STREQUAL "minimal") | |||
| CONFIG_ECMA_COMPACT_PROFILE_DISABLE_JSON_BUILTIN) | |||
|
|
|||
| else() | |||
| message(FATAL_ERROR "FEATURE_PROFILE='${FEATURE_PROFILE}' doesn't supported") | |||
There was a problem hiding this comment.
Can't we use elseif like above?
There was a problem hiding this comment.
Sure, I've updated the patch.
robertsipka
force-pushed
the
ecma_number_type_value
branch
from
81475c3 to
6ce67c1
Compare
|
LGTM |
| set(DEFINES_JERRY | ||
| ${DEFINES_JERRY} | ||
| CONFIG_ECMA_COMPACT_PROFILE | ||
| CONFIG_ECMA_NUMBER_TYPE=CONFIG_ECMA_NUMBER_FLOAT32) |
There was a problem hiding this comment.
Was this line added by mistake? (CONFIG_ECMA_NUMBER_TYPE=CONFIG_ECMA_NUMBER_FLOAT32)
This would give us:
- Minimal profile: FLOAT64
- Compact profile: FLOAT32
- Full profile: FLOAT64
I think we want the default to be FLOAT64 in all three profiles :)
|
LGTM (minus feedback above) Can you please mention in the commit message that we're changing the default to FLOAT64 to ensure compliance with ECMAScript 5.1 which requires numbers to be represented in double precision floating-point format? I'm thinking we should not expose FLOAT32 as a CMake option to have a higher barrier to ensure that only experienced users who understand the performance/precision trade-off of going with FLOAT32 (and are willing to sacrifice ECMAScript 5.1 conformance) will actually use this. |
… compliance with ECMAScript 5.1 which requires numbers to be represented in double precision floating-point format. JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com
robertsipka
force-pushed
the
ecma_number_type_value
branch
from
6ce67c1 to
15ba287
Compare
|
@tilmannOSG : Thanks, I've updated this patch based on your comments. |
|
@robertsipka Thanks, looks good! |
|
LGTM |
…R_FLOAT64.
JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com