erichkeane added a comment.
Sorry for the long delay, the size of this makes it tough to review, and I've
been trying to make sure my concepts patch didn't get reverted. This generally
looks good to me, however, I really don't like the number of bits to represent
'template parameter' being inconsistent between bitfields in general. I'm
reasonably OK limiting it, but having 'surprise' limits only when we deal with
replacements isn't really acceptable. When we hit these limits, we should fail
early.
================
Comment at: clang/include/clang/AST/Type.h:1838
/// metaprogramming we'd prefer to keep it as large as possible.
- /// At the moment it has been left as a non-bitfield since this type
- /// safely fits in 64 bits as an unsigned, so there is no reason to
- /// introduce the performance impact of a bitfield.
- unsigned NumArgs;
+ unsigned NumArgs : 16;
};
----------------
mizvekov wrote:
> davrec wrote:
> > I can't imagine that limiting template arg index to 16 bits from 32 could
> > be all that limiting, but given the comment in the original have you
> > tested/confirmed that this is acceptable?
> Not yet, we will begin performance testing soon. But I am not concerned about
> this one, as it's easy to get around this limitation by not using the
> bitfields.
Did the testing here result in finding anyone who used this? I'm sure libcxx
or some of the boost libraries do a lot of MP on large sizes (though I note
libcxx seems to have passed pre-commit).
================
Comment at: clang/include/clang/AST/Type.h:1836
+ // The index of the template parameter this substitution represents.
+ unsigned Index : 15;
+
----------------
Is it a problem for this to be a different number of bits used to represent the
number of template parameters? I would expect we would want to make them all
them the same size (else we have an additional limit on the size of parameters).
================
Comment at: clang/include/clang/Sema/Template.h:104
/// Construct a single-level template argument list.
- explicit
- MultiLevelTemplateArgumentList(const TemplateArgumentList &TemplateArgs) {
- addOuterTemplateArguments(&TemplateArgs);
+ explicit MultiLevelTemplateArgumentList(Decl *D, ArgList Args) {
+ addOuterTemplateArguments(D, Args);
----------------
2 parameters with no default means this doesn't have to be explicit anymore.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131858/new/
https://reviews.llvm.org/D131858
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
