Add another argument for the JERRY_STATIC_ASSERT with the description of the assert statement. #916
Conversation
robertsipka
force-pushed
the
build_fix
branch
from
0d83ff0 to
958664e
Compare
|
I got confused by the title and the comment of this PR when I took a look at the patch. All I saw was that static asserts were extended with an additional message-like argument. I'm still not sure why that's needed. What kind of typedefs get redefined? Other than that, the patch seems to be a fair step-by-step rewrite of all static asserts, that's good. (If that's really needed.) Finally, please make sure that all touched files have the copyright year(s) updated in their headers. (At least some of them contain 2015.) |
| JERRY_STATIC_ASSERT (sizeof (ecma_object_t) <= sizeof (uint64_t), | ||
| size_of_ecma_object_t_must_be_less_than_or_equal_to_8_bytes); | ||
| JERRY_STATIC_ASSERT (ECMA_OBJECT_OBJ_TYPE_SIZE <= sizeof (uint64_t) * JERRY_BITSINBYTE, | ||
| ECMA_OBJECT_OBJ_TYPE_SIZE_must_be_less_than_or_equal_to_64_bytes); |
|
The generic direction is good, but I also agree with Akos that some names need to be a bit better. |
robertsipka
force-pushed
the
build_fix
branch
from
958664e to
18a1a64
Compare
|
Thanks for the comments, I've updated the patch with the suggested names. |
| JERRY_STATIC_ASSERT (sizeof (ecma_object_t) <= sizeof (uint64_t), | ||
| size_of_ecma_object_t_must_be_less_than_or_equal_to_8_bytes); | ||
| JERRY_STATIC_ASSERT (ECMA_OBJECT_OBJ_TYPE_SIZE <= sizeof (uint64_t) * JERRY_BITSINBYTE, | ||
| ECMA_OBJECT_OBJ_TYPE_SIZE_must_be_less_than_or_equal_to_bits_in_uint64_t); |
There was a problem hiding this comment.
ECMA_OBJECT_OBJ_TYPE_SIZE_must_be_less_than_or_equal_to_64_bits ? A bit shorter.
|
Patch looks much better. |
robertsipka
force-pushed
the
build_fix
branch
from
18a1a64 to
da53dcc
Compare
|
Thanks, I've updated, and I've also removed the duplicated static asserts too. |
| @@ -1,4 +1,5 @@ | |||
| /* Copyright 2014-2015 Samsung Electronics Co., Ltd. | |||
There was a problem hiding this comment.
/* Copyright 2014-2016 Samsung Electronics Co., Ltd.
|
Patch looks good, thanks. But still, could you please provide a description of the error it fixes? At least for future reference. |
robertsipka
force-pushed
the
build_fix
branch
from
da53dcc to
de570bd
Compare
|
@akiss77 : The JERRY_STATIC_ASSERT defines a variable, where the name contains the __ LINE __ of the assert. So, in case of all-in-one build, it's possible that there are at least two variables which have the same name (redefinition), because an assert occurs in the same line in another file. |
|
@robertsipka thanks, LGTM (FWIW) |
|
LGTM |
robertsipka
force-pushed
the
build_fix
branch
from
de570bd to
09ac125
Compare
robertsipka
changed the title
robertsipka
force-pushed
the
build_fix
branch
from
09ac125 to
ae9974a
Compare
|
LGTM |
Add another argument for the JERRY_STATIC_ASSERT with the description of the assert statement. The release.linux build fails with enabled ALL_IN_ONE option. There is no redefinition of typedef in C99. JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com
robertsipka
force-pushed
the
build_fix
branch
from
ae9974a to
af715d4
Compare
The release.linux build fails with enabled ALL_IN_ONE option.