[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via cfe-commits]

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