[PATCH] D151952: [clang] adds `__type_pack_index` so we can get a type's parameter pack index

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Needs a release note.

Also, is this something that has been requested by library?  I'd hope this is something that will be used, so I'd like evidence of that.



================
Comment at: clang/include/clang/AST/Stmt.h:796-802
+    /// If this expression is not value-dependent, this stores the value.
+    unsigned Value : 8;
 
     /// The number of arguments to this type trait. According to [implimits]
     /// 8 bits would be enough, but we require (and test for) at least 16 bits
     /// to mirror FunctionType.
+    unsigned NumArgs : 16;
----------------
dblaikie wrote:
> cjdb wrote:
> > These numbers feel very low for how this modification is using them. Perhaps they should both be bumped to 32?
> clang does, I think, have specific implementation limits for this sort of thing - 
> 
> Well, maybe we don't actually enforce a limit on number of template arguments... https://godbolt.org/z/accncYson compiles successfully, and has 2^18 parameters, beyond the 2^16 suggested here.
> 
> But maybe 2^16 is just what we test for/somewhat guarantee.
> 
> But if the `Value` is going to be the index into the args, shouldn't it be the same size as `NumArgs`, at least? (& the comment says 16, so 16 for both Value and NumArgs would seem suitably symmetric, and no larger than the current situation - since it'd just be splitting NumArgs in half, while still meeting the comment's suggestion/requirements?)
> 
> An assert, at least, when populating NumArgs to ensure it's no larger might not hurt... (which might be reachable from code/not checked prior - so it could be a hard compilation error, I guess, but I wouldn't feel super strongly about it)
Can you explain the math here of how you chose to change this to 16?  I see that you removed 7 bits, but took away 16.  I'm not sure what NumExprBits is doing though, I got lost a little in that, so if you can calculate that to make sure this needs to be done, it would be appreciated.

Additionally, a static_assert after this to ensure the size isn't changing would also be appreciated.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2960
+def err_type_pack_index_not_found : Error<
+  "'__type_pack_index' couldn't find type %0">;
+
----------------



================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5562
+  if (IsDependent)
+    goto Return;
+
----------------
Oh, please don't do this.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5591
+  default:
+    llvm_unreachable("unhandled type trait (usualy deliberate)");
+  }
----------------
What do you mean by `usually deliberate` here? This is a message users will see, so telling them an assertion is deliberate seems incorrect? 


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5620
 
+  if (Kind == clang::TT_TypePackIndex)
+    return EvaluateIntegralTypeTrait(*this, Kind, KWLoc, Args, RParenLoc,
----------------
I realize this is the 1st, but this seems like it is going to be a maintenance pain.  Can you at least split this into a static-function of `IsIntegralTypeTrait` that we can use later with table-gen if this gets out of hand?


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5625
   bool Result = false;
   if (!Dependent)
+    Result = EvaluateBooleanTypeTrait(*this, Kind, KWLoc, Args, RParenLoc);
----------------
For the purposes of making the two the same, can you integrate this into `EvaluateBooleanTypeTrait`?  That would be something like:

```
if (IsIntegralTypeTrait(Kind))
  return EvaluateIntegralTypeTrait(...);
return EvaluateBooleanTypeTrait(...);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151952