[PATCH] D100672: [ADT] Add new type traits for type pack indexes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:151-154
+  for (size_t I = 1; I <= sizeof...(Ts); ++I)
+    if (Matches[I])
+      return I - 1;
+  return sizeof...(Ts);
----------------
scott.linder wrote:
> dblaikie wrote:
> > Could this be replaced with:
> > ```
> > return llvm::find(Matches, true) - std::begin(Matches);
> > ```
> > or something like that?
> > (similarly below in "AreTypesDistinct")?
> > 
> > Hmm, what's the purpose of the first element of the array being 'false', and then iterating from the second element and subtracting one from the index before returning?
> > 
> > Oh, that's to support the zero-length case? Fair enough. I don't mind either way, but maybe it'd be simpler/more obvious if that were done via specialization? - oh, what happens if you put the buffer value at the end rather than the beginning. In this algorithm you could put "true" at the end (and remove "false" from the start) and then it'd return 1 when sizeof...(Ts) is zero?
> > 
> Sure, I think `llvm::find` would need to be constexpr then? I assume that will ripple through some other things in `STLExtras.h`, so maybe I should do a pass to add `constexpr` wherever possible in `STLExtras.h`? Or in this case would just marking what I need be better?
> 
> Putting the extra element at the end and having it defined to return a size >= sizeof...(Ts) seems reasonable, especially as it is just a detail function. I'll make that change, thank you!
oh, sure if there's stuff that needs to be constexpr and isn't - if it's not a ton of stuff, can probably include it in this patch (adding the constexpr keyword as needed) - if it's a lot, maybe a separate preliminary patch. (I probably wouldn't bother adding unit tests for constexpr-ness, but treat it as a quasi-NFC change). (if you want to include only what's needed in this patch and also separately submit a patch to constexpr the rest (then maybe some testing may be needed, to ensure all the implementation details got constexpr'd correctly))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100672