sammccall added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5092 "expression not permitted as operand of fold expression">; +def err_fold_expression_expansion_exceeded: Error< + "fold expression expansion level %0 exceeded maximum of %1">; ---------------- I think this should be NoSFINAE, similar to err_template_recursion_depth_exceeded. Maybe DefaultFatal too? ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5093 +def err_fold_expression_expansion_exceeded: Error< + "fold expression expansion level %0 exceeded maximum of %1">; ---------------- The diagnostic test could be clearer: what's a "fold expression expansion level"? what about "instantiating fold expression with %0 arguments exceeds expression nesting limit of %1"? ================ Comment at: clang/lib/Sema/TreeTransform.h:13195 } + if (NumExpansions && SemaRef.getLangOpts().BracketDepth < NumExpansions) { + SemaRef.Diag(E->getEllipsisLoc(), ---------------- The brackets here are not obvious. Also we're not actually tracking bracket *depth*! So this definitely needs a comment. Something like: ``` // Formally a fold expression expands to nested parenthesized expressions. // Enforce this limit to avoid creating trees so deep we can't safely traverse them ``` ================ Comment at: clang/lib/Sema/TreeTransform.h:13196 + if (NumExpansions && SemaRef.getLangOpts().BracketDepth < NumExpansions) { + SemaRef.Diag(E->getEllipsisLoc(), + clang::diag::err_fold_expression_expansion_exceeded) ---------------- you might also want to emit note_bracket_depth (I don't think you need to clone it if we get the wording right) ================ Comment at: clang/test/SemaCXX/fold_expr_expansion_limit.cpp:8 +auto x = __make_integer_seq<seq, int, N>{}; +static_assert(!x.zero(), ""); // expected-error {{static_assert expression is not an integral constant expression}} \ + expected-note {{in instantiation of member function}} ---------------- this diagnostic is bogus :-( default-fatal would fix this, I guess. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86936/new/ https://reviews.llvm.org/D86936 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits