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