Set length variable for slice and index expressions during semantic#7682
Set length variable for slice and index expressions during semantic#7682dlang-bot merged 1 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request, @ibuclaw! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
|
@nordlow - we can do this at least. :-) |
|
@ibuclaw - great |
|
closed and reopened to restart SemaphoreCI (after moving it to my account, s.t. @ibuclaw can login there via SSH too). If you want to be able to do the same, login once via GitHub at Semaphore and then let me know. |
wilzbach
left a comment
There was a problem hiding this comment.
Shouldn't there be tests in fail_compilation for cases where constant folded detected an error?
| if (!lengthVar) | ||
| return; | ||
| if (lengthVar._init && !lengthVar._init.isVoidInitializer()) | ||
| return; // we have previously calculated the length |
There was a problem hiding this comment.
Apparently this caching doesn't work.
There was a problem hiding this comment.
You mean coverage wise? That's probably because semantic only calls it once. Doesn't make the safeguards any less correct.
| else | ||
| { | ||
| Type t = arr.type.toBasetype(); | ||
| if (t.ty == Tsarray) |
There was a problem hiding this comment.
Looks like this doesn't work.
There was a problem hiding this comment.
Control flow has likely moved this to the other function. Should still be kept around though, as it's a utility function.
test/runnable/testbounds.d
Outdated
|
|
||
| { auto s = da[l .. u]; } // 0 0 (u <= 10 && l <= u) | ||
| { auto s = da[0 .. u]; } // 0 1 (u <= 10 ) | ||
| { auto s = da[l .. 10]; } // 0 0 (u <= 10 && l <= u) |
test/runnable/testbounds.d
Outdated
| size_t u = 9; // | lowerLessThan | ||
| // | | check code | ||
| { auto s = da[l .. u]; } // 0 0 (u <= 10 && l <= u ) | ||
| { auto s = da[1 .. u]; } // 0 0 (u <= 10 && l <= u ) |
There was a problem hiding this comment.
I should probably change my font, because all I see is the same character twice in your comment. I assume you mean there's a mixup between number (1) and letter (L)?
There was a problem hiding this comment.
Yes, the non-monospace font shows it more clearly:
- { auto s = da[l .. u]; } // 0 0 (u <= 10 && l <= u )
- { auto s = da[1 .. u]; } // 0 0 (u <= 10 && l <= u )
There was a problem hiding this comment.
I was trying to figure out why that was even compiling. Line 190 shows size_t l = 0; // upperInRange, so it looks ok, though it could probably use a better name.
test/runnable/testbounds.d
Outdated
| { auto s = da[l .. u]; } // 0 0 (u <= 10 && l <= u ) | ||
| { auto s = da[1 .. u]; } // 0 0 (u <= 10 && l <= u ) | ||
| { auto s = da[l .. 10]; } // 0 0 (u <= 10 && l <= u ) | ||
| { auto s = da[1 .. u%5]; } // 0 0 (u <= 10 && l <= u%5) |
|
(rebased with trivial conflicts) |
9884a2d to
a85ab8c
Compare
|
Rebased, clarified names of vars in the bounds tests. |
| { auto s = da[0 .. ub%5]; } // 0 1 (ub%5 <= $ ) | ||
|
|
||
| { auto s = da[0 .. 0]; } // 0 1 (0 <= $ ) | ||
| { auto s = da[0 .. $]; } // 0 1 ($ <= $ ) |
There was a problem hiding this comment.
These two can be improved in a latter PR by using the check for 0 or $ from #7677.
|
Closed and reopened in an effort to get Jenkins to pick up the latest bits. |
|
Really need to fix that... |
|
Hopefully dlang/ci#163 helps a bit. |
|
Rebased, again. |
Optimization opportunity noticed in #7677.
For static arrays, slice and indexes using values known at compile time should now never generate array bounds checks at codegen.
Before:
After
For reviewing the test changes (mostly just whitespace alignment with the rest of the function).
https://github.com/dlang/dmd/pull/7682/files?w=1