cjdb 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;
----------------
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).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151952/new/
https://reviews.llvm.org/D151952
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
