[PATCH] D103900: [llvm] Add enum iteration to Sequence
Guillaume Chatelet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 5 07:46:37 PDT 2021
gchatelet added inline comments.
================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceAttributes.cpp:87-91
+ // `index_begin` is unsigned(~0), so effectively iterating ~0, 0, 1, 2...
+ // This is why we need to use `SetIdx != EndIdx` instead of `SetIdx <
+ // EndIdx`.
+ for (unsigned SetIdx = AL.index_begin(), EndIdx = AL.index_end();
+ SetIdx != EndIdx; ++SetIdx) {
----------------
lebedev.ri wrote:
> Quuxplusone wrote:
> > LGTM, but per @lebedev.ri 's comment in D83351, I think this would benefit from a follow-up patch that turns `index_begin` and `index_end` into `signed int`s.
> > Style nit: Personally I'd rather see "`unsigned(-1)`" and "`-1, 0, 1...`", than "`unsigned(~0)`" and "`~0, 0, 1...`".
> This change NACK.
> It should probably be something like
> ```
> for (unsigned SetIdx : seq<int>(AL.index_begin(), AL.index_end())) {
> ```
> or
> ```
> for (unsigned SetIdx : seq((int)AL.index_begin(), (int)AL.index_end())) {
> ```
Ideally, `AttributeList` should be changed to preserve iterator semantic, this was the intention of one of the previous author (Read summary of D32811).
Unfortunately it seems a bit more work than just [[ https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/Attributes.h#L387-L391 | renaming the enum ]].
I went for the very explicit version because using `int` involves two casts:
- two from unsigned to signed which are implementation defined and thus non standard.
- one from signed to unsigned which is well defined and uses modulo 2 arithmetic.
See **Integer conversions** in https://en.cppreference.com/w/c/language/conversion
I //think// it's important to make sure the code is crystal clear and the double cast makes it non trivial to reason about.
We can:
# Use `for (unsigned SetIdx : seq<int>(AL.index_begin(), AL.index_end()))` although I think it's implementation defined and a fix for something that is broken elsewhere.
# Write the loop explicitly explaining why we have to do it this way, allegedly not fixing the real problem either.
# Invest extra time to fix the issue in `AttributeList` so it respects `begin<=end`.
WDYT?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103900/new/
https://reviews.llvm.org/D103900
More information about the llvm-commits
mailing list