rsmith added a comment. I wonder if there are any cases where the "deduction guide" is so broken that we shouldn't be creating a `CXXDeductionGuideDecl` at all. For example, if there's no trailing return type, then the declaration is syntactically invalid (and looks more like a constructor than a deduction guide), and it's not clear to me whether creating a `CXXDeductionGuideDecl` really makes sense.
In D149516#4308203 <https://reviews.llvm.org/D149516#4308203>, @shafik wrote: > I have some questions, why don't we always call > `CheckDeductionGuideDeclarator` when calling `CXXDeductionGuideDecl::Create`, > I felt like perhaps `CheckDeductionGuideDeclarator` should be rolled into > `CXXDeductionGuideDecl::Create` but then I noticed they are not paired up in > other locations. `CXXDeductionGuideDecl::Create` is the low-level AST primitive for creating a `CXXDeductionGuideDecl`. The responsibility for validating the declaration and issuing diagnostics doesn't belong at that level, it belongs in `Sema`. The other callers are: - When `Sema` creates an implicit deduction guide from a constructor. In this case `Sema` knows it's valid by construction. - When `Sema` instantiates a deduction guide. In this case `Sema` knows it's valid because the template was. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:9202 << TrailingRequiresClause->getSourceRange(); - SemaRef.CheckDeductionGuideDeclarator(D, R, SC); + bool IsValid = SemaRef.CheckDeductionGuideDeclarator(D, R, SC); ---------------- This is reversed from the normal Clang convention (where `true` indicates that an error was produced). ================ Comment at: clang/lib/Sema/SemaDecl.cpp:9204 - return CXXDeductionGuideDecl::Create(SemaRef.Context, DC, D.getBeginLoc(), + CXXDeductionGuideDecl* DGD = CXXDeductionGuideDecl::Create(SemaRef.Context, DC, D.getBeginLoc(), ExplicitSpecifier, NameInfo, R, TInfo, ---------------- Use `auto` here. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11141-11145 if (!Chunk.Fun.hasTrailingReturnType()) { + IsValid = false; Diag(D.getName().getBeginLoc(), diag::err_deduction_guide_no_trailing_return_type); break; ---------------- Can we just return immediately here? I don't see a reason to keep going -- this error may well indicate that the developer didn't actually mean to declare a deduction guide in the first place, so the diagnostic on a function body below is likely to not be useful. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11196 if (D.isFunctionDefinition()) Diag(D.getIdentifierLoc(), diag::err_deduction_guide_defines_function); + return IsValid; ---------------- Should we return that we had an error after this one too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149516/new/ https://reviews.llvm.org/D149516 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
