[PATCH] D103900: [llvm] Add enum iteration to Sequence

[Guillaume Chatelet via Phabricator via llvm-commits]

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) {
----------------
Quuxplusone wrote:
> gchatelet wrote:
> > 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?
> I approve of `seq((int)AL.index_begin(), (int)AL.index_end())` but not `seq<int>(AL.index_begin(), AL.index_end())` — I don't want people writing explicit template arguments all over the place.
> Regardless, this would still benefit from a followup patch to actually change the type of `index_begin` so that we don't //have// to cast it on every use.
Since I'm blocked on D105485, I'll submit this one using the idiomatic way of iterating through `AttributeList`. This is consistent with what I did in [[ https://reviews.llvm.org/D105485#change-KSS4Eh8MkRee | D105485 ]] as well.

```
 % git grep \.index_begin\(
include/llvm/IR/Attributes.h:764:  unsigned index_begin() const { return AttributeList::FunctionIndex; }
lib/Bitcode/Writer/BitcodeWriter.cpp:836:    for (unsigned i = AL.index_begin(), e = AL.index_end(); i != e; ++i) {
lib/Bitcode/Writer/ValueEnumerator.cpp:1039:  for (unsigned i = PAL.index_begin(), e = PAL.index_end(); i != e; ++i) {
lib/IR/Attributes.cpp:1550:  for (unsigned i = index_begin(), e = index_end(); i != e; ++i) {
lib/Transforms/Utils/FunctionComparator.cpp:113:  for (unsigned i = L.index_begin(), e = L.index_end(); i != e; ++i) {
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103900/new/

https://reviews.llvm.org/D103900