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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits