[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.
Herald added a subscriber: arphaman.

This looks great, thanks!

In https://reviews.llvm.org/D36527#892395, @hamzasood wrote:

> Could you expand on your first point a bit more? Do you have an example that shows the issue?


You have very little test coverage for the case where a lambda with explicit template parameters is used within another template. Eg, something like (untested):

  template<typename T> constexpr T f() {
    return []<T V>() { return V; }.operator()<12345>(); // expected-error {{no viable function}} expected-note {{candidate}}
  }
  static_assert(f<int>() == 12345); // ok
  int *p = f<int*>(); // expected-note {{in instantiation of}}



================
Comment at: lib/AST/DeclCXX.cpp:1396-1397
+
+  return std::count_if(List->begin(), List->end(),
+                       [](const NamedDecl *D) { return !D->isImplicit(); });
+}
----------------
Given that you've already assert-checked `is_partitioned`, maybe use a binary search here (eg, `lower_bound`)?


================
Comment at: lib/AST/DeclPrinter.cpp:107-108
 
-    void printTemplateParameters(const TemplateParameterList *Params);
+    void printTemplateParameters(const TemplateParameterList *Params,
+                                 bool OmitTemplateKW = false);
     void printTemplateArguments(const TemplateArgumentList &Args,
----------------
Nit: I'd prefer splitting this into two functions (one that prints 'template', calls the other, then prints a space, and the other to print the actual template parameters) rather than passing a boolean flag.


================
Comment at: lib/AST/ItaniumMangle.cpp:1710
+    Out << "Ty";
+  }
+  else if (auto *Tn = dyn_cast<NonTypeTemplateParmDecl>(Decl)) {
----------------
No newline between `}` and `else`, please.


================
Comment at: lib/AST/ItaniumMangle.cpp:1752-1757
+    const auto *List = Lambda->getGenericLambdaTemplateParameterList();
+    assert(List && "Lambda says it has explicit template parameters, "
+                   "but it doesn't have a template parameter list");
+    for (auto It = List->begin(), End = It + ExplcitTemplateParamCount;
+         It != End;
+         ++It)
----------------
Might be nice to add an accessor that returns the range of explicit parameters. (The `if` surrounding this code would then also be redundant.)


================
Comment at: lib/Parse/ParseExprCXX.cpp:1139
+                                TemplateParams, LAngleLoc, RAngleLoc)) {
+      return ExprError();
+    }
----------------
You need to do something here to leave the lambda scope that was pushed near the start of this function.


================
Comment at: unittests/AST/StmtPrinterTest.cpp:109
 
-::testing::AssertionResult
-PrintedStmtCXX98Matches(StringRef Code, const StatementMatcher &NodeMatch,
-                        StringRef ExpectedPrinted) {
-  std::vector<std::string> Args;
-  Args.push_back("-std=c++98");
-  Args.push_back("-Wno-unused-value");
-  return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted);
-}
-
-::testing::AssertionResult PrintedStmtCXX98Matches(
-                                              StringRef Code,
-                                              StringRef ContainingFunction,
-                                              StringRef ExpectedPrinted) {
-  std::vector<std::string> Args;
-  Args.push_back("-std=c++98");
-  Args.push_back("-Wno-unused-value");
-  return PrintedStmtMatches(Code,
-                            Args,
-                            functionDecl(hasName(ContainingFunction),
-                                         has(compoundStmt(has(stmt().bind("id"))))),
-                            ExpectedPrinted);
+enum class StdVer { CXX98, CXX11, CXX14, CXX17, CXX2a };
+
----------------
Please commit the refactoring portion of the changes to this file separately.


https://reviews.llvm.org/D36527