[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 inline comments.


================
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;
----------------
cjdb wrote:
> erichkeane wrote:
> > 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.
> > 
> > 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?)
> 
> Someone independently confirmed that you can have 200k types in a template, so we should probably be able to count at least that high (or alternatively, we should possibly consider not allowing more than 2^16 template parameters).
> 
> > 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.
> 
> The value of `NumArgs` hasn't changed: I've just codified it. I can't work out why we need `NumExprBits`, but it's apparently required padding (when I removed it, a bunch of tests failed). Its value is 18, courtesy of clangd in VS Code.
> 
> > Additionally, a static_assert after this to ensure the size isn't changing would also be appreciated.
> 
> There's a static_assert in one of the `Stmt` constructors, which doesn't want more than eight bytes (we apparently need 16 if we're to change this).
Got it, thanks!  We need the `NumExprBits` because this gets cast to that `ExprBitFields` (since this inherits from Expr).  So thats the 'base type' bits.

18 + 8 + 8 (for the `NumExprbits` and `Kind` and `Value` fields) is 34, so that leaves plenty of extra bits here, right?  Usually in these cases we leave things 'as big as we can without growing the thing', then comment where extra bits can be stolen in the future.

So I would just make this 'the rest' of our size, since we're already over 4 bytes, mind as well use them as long as we can.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151952