[PATCH] D13786: [Sema] Implement __make_integer_seq

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/TemplateName.h:93-100
@@ +92,10 @@
+
+/// \brief A structure for storing the information associated with an
+/// builtin template name.
+class BuiltinTemplateStorage : public UncommonTemplateNameStorage {
+  friend class ASTContext;
+
+public:
+  BuiltinTemplateStorage() : UncommonTemplateNameStorage(Builtin, 0) {}
+};
+
----------------
Please add an enum for builtin template names (and pass it around when creating `TemplateName`s and `BuiltinTemplateDecl`s as appropriate), even though it will only have one value for now.

================
Comment at: lib/Parse/ParseDecl.cpp:2937-2939
@@ +2936,5 @@
+      if (Type.isInvalid()) {
+        // If we failed to parse the template ID but skipped ahead to a >, we're
+        // not
+        // going to be able to form a token annotation.  Eat the '>' if present.
+        TryConsumeToken(tok::greater);
----------------
Reflow this comment.

================
Comment at: lib/Sema/SemaTemplate.cpp:2060-2102
@@ -2055,1 +2059,45 @@
 
+  // Instantiations involving __make_integer_seq<S, T, N> are treated like
+  // S<T, 0, ..., N-1>.
+  TemplateArgumentListInfo SyntheticTemplateArgs;
+  TemplateName UnderlyingName = Name;
+  bool UseSyntheticTemplateArgs = false;
+  if (IsBuiltinTemplate) {
+    assert(Converted.size() == 3 && "Unexpected BuiltinTemplate!");
+
+    // Switch our template from __make_integer_seq to S.
+    Name = Converted[0].getAsTemplate();
+    Template = Name.getAsTemplateDecl();
+    TemplateArgument NumArgsArg = Converted[2];
+    if (!NumArgsArg.isDependent()) {
+      // The first template argument will be reused as the template decl that
+      // our synthetic template arguments will be applied to while the last
+      // template argument will be replaced with a pack.
+      TemplateArgument SequenceType = Converted[1];
+      Converted.clear();
+      Converted.push_back(SequenceType);
+      UseSyntheticTemplateArgs = true;
+
+      // Diagnose attempts to create integer sequences with a negative length.
+      llvm::APSInt NumArgs = NumArgsArg.getAsIntegral();
+      if (NumArgs < 0)
+        Diag(UnderlyingTemplateArgs[2].getLocation(),
+             diag::err_integer_sequence_negative_length);
+
+      QualType Ty = NumArgsArg.getIntegralType();
+      SmallVector<TemplateArgument, 2> ArgumentPack;
+      for (llvm::APSInt I(NumArgs.getBitWidth(), NumArgs.isUnsigned());
+           I < NumArgs; ++I) {
+        TemplateArgument TA(Context, I, Ty);
+        Expr *E =
+            BuildExpressionFromIntegralTemplateArgument(TA, SourceLocation())
+                .getAs<Expr>();
+        ArgumentPack.push_back(TA);
+        SyntheticTemplateArgs.addArgument(
+            TemplateArgumentLoc(TemplateArgument(E), E));
+      }
+      Converted.push_back(
+          TemplateArgument::CreatePackCopy(Context, ArgumentPack));
+    }
+  }
+
----------------
Please factor this out into a separate function, `CheckBuiltinTemplateIdType` or similar.

================
Comment at: lib/Sema/SemaTemplate.cpp:2069-2071
@@ +2068,5 @@
+    // Switch our template from __make_integer_seq to S.
+    Name = Converted[0].getAsTemplate();
+    Template = Name.getAsTemplateDecl();
+    TemplateArgument NumArgsArg = Converted[2];
+    if (!NumArgsArg.isDependent()) {
----------------
I would prefer to see this structured as follows:

In the `if` chain below where we're dispatching on the type of the template, if the template is a `BuiltinTemplateDecl`, then do some special-case handling to compute the `CanonType` (which is really poorly / misleadingly named; it's just the underlying type, not the canonical type). In the `__make_integer_sequence` case, this would involve recursively calling back into `CheckTemplateIdType` to build the template-id for the expanded form.

This means you'll retain an intermediate TST between the `__make_integer_seq` and the ultimate type.

================
Comment at: lib/Sema/SemaTemplate.cpp:2083-2085
@@ +2082,5 @@
+      llvm::APSInt NumArgs = NumArgsArg.getAsIntegral();
+      if (NumArgs < 0)
+        Diag(UnderlyingTemplateArgs[2].getLocation(),
+             diag::err_integer_sequence_negative_length);
+
----------------
Is it worth putting some kind of upper bound on what we'll accept here?


http://reviews.llvm.org/D13786