Issue 9766 - align(n) with n compile-time constant#5750
Issue 9766 - align(n) with n compile-time constant#57509rnsr wants to merge 1 commit intodlang:masterfrom
Conversation
|
|
Note that, under the current design of |
|
PR for language spec: dlang/dlang.org#1293 |
src/attrib.d
Outdated
| return STRUCTALIGN_DEFAULT; | ||
|
|
||
| auto n = ealign.toInteger(); | ||
| if (!tb.isunsigned() && n > long.max) |
There was a problem hiding this comment.
Should be simply:
n >= 1 && n < structalign_t.max;
There was a problem hiding this comment.
No, n is dinteger_t, it's unsigned.
And when n is bigger than structalign_t.max, currently it's silently accepted.
Line 981 in d28f6bb
Should I add a new error check in here?
There was a problem hiding this comment.
No, n is dinteger_t , it's unsigned.
The signed-ness is actually irrelevant for the test. Your line also doesn't check for 0, and it's absurd to have alignments greater than long.max.
There was a problem hiding this comment.
Please read my repply. Currently alignment value greater than long.max is silently accepted. Your proposal would be legitimiate, but it's unrelated with the new feature implementation. Shouldn't it go to separated bugfix PR?
There was a problem hiding this comment.
And, lack of check for 0 is my mistake. Will fix.
There was a problem hiding this comment.
Shouldn't it go to separated bugfix PR?
Replicating an extremely minor bug just so it can be fixed later doesn't seem worthwhile.
There was a problem hiding this comment.
Bug is bug. You've been rejected to do two things in a PR, so I follow the principle in here. If the bugfix is important, please file it to bugzilla by your hand.
There was a problem hiding this comment.
I'm really sorry, I was wrong. Even in current master, alignment value greater than structalign_t.max is rejected.
In
Line 977 in d28f6bb
By the test token.value == TOKint32v, the upper boundary of parsed integer literal has been limited to uint.max...
I updated code to follow Walter's suggestion.
src/attrib.d
Outdated
| if (!tb.isunsigned() && n > long.max) | ||
| return showError(); | ||
|
|
||
| salign = cast(structalign_t)n; |
There was a problem hiding this comment.
The downcast silently chops any values between stackalign_t.max and long.max. Please use the formula I gave, which won't do that.
|
I fix most of issues pointed out by @WalterBright. |
9rnsr
force-pushed
the
fix9766
branch
4 times, most recently
from
4178c61 to
60e6c7c
Compare
|
@9rnsr So amazing your quick play! I thik that we could use zero, -1, 0xffff, or something like ALIGN_DEFAULT. Anyways, thank you for your favor. |
|
comment deleted because it was wrong |
|
|
||
| sc = sc.startCTFE(); | ||
| ealign = ealign.semantic(sc); | ||
| ealign = resolveProperties(sc, ealign); |
There was a problem hiding this comment.
seems to me ctfeInterpret() should be put here.
There was a problem hiding this comment.
and then just do toInteger(), which will take care of most of the cases below. Then, just have one:
n >= 1 && n <= structalign_t.max && !(n & (n - 1))
should do it.
|
Rebased and rebooted as #5880 |
4 participants
Implement the feature.