dblaikie 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:
> 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)
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
