aaron.ballman added a comment. I really like this attribute, thank you for working on it!
================ Comment at: include/clang/Basic/Attr.td:1584 +def DiagnoseIf : InheritableAttr { + let Spellings = [GNU<"diagnose_if">]; + let Subjects = SubjectList<[Function]>; ---------------- I think this should also have a C++ spelling under the clang namespace. ================ Comment at: include/clang/Basic/AttrDocs.td:359 + int val3 = abs(val); + int val4 = must_abs(val); // because run-time checks are not emitted for + // diagnose_if attributes, this executes without ---------------- Capitalize the B in because. ================ Comment at: include/clang/Basic/DiagnosticCommonKinds.td:164 InGroup<GccCompat>; +def ext_clang_diagnose_if : Extension<"'diagnose_if' is a clang extension">, + InGroup<GccCompat>; ---------------- If we do add the clang namespaced spelling, we should have a test to ensure that this diagnostic is not triggered when the attribute is spelled `[[clang::diagnose_if(...)]]`. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2145 +def err_diagnose_if_invalid_diagnostic_type : Error< + "invalid diagnostic type for diagnose_if. Try 'error' or 'warning'.">; def err_constexpr_body_no_return : Error< ---------------- Diagnostics don't have full stops, so I think a better way to write this is: `invalid diagnostic type for 'diagnose_if'; use \"error\" or \"warning\" instead`. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3361 "pass_object_size attribute">; -def note_ovl_candidate_disabled_by_enable_if_attr : Note< +def err_diagnose_if_succeeded: Error<"%0">; +def warn_diagnose_if_succeeded : Warning<"%0">, InGroup<UserDefinedWarnings>; ---------------- Space before the colon. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4380 InGroup<DeprecatedDeclarations>; +def note_from_diagnose_if : Note<"from diagnose_if attribute on %0:">; def warn_property_method_deprecated : ---------------- Please single quote the use of diagnose_if. ================ Comment at: include/clang/Sema/Overload.h:664 + + /// Basically an TinyPtrVector<DiagnoseIfAttr *> that doesn't own the + /// vector: If NumTriggeredDiagnoseIfs is 0 or 1, this is a DiagnoseIfAttr ---------------- an -> a ================ Comment at: include/clang/Sema/Sema.h:2616 + DiagnoseIfAttr * + checkArgDependentDiagnoseIf(FunctionDecl *Function, ArrayRef<Expr *> Args, + SmallVectorImpl<DiagnoseIfAttr *> &Nonfatal, ---------------- Can this (and the other new functions) accept the `FunctionDecl` as a `const FunctionDecl *` instead? ================ Comment at: lib/Sema/SemaDeclAttr.cpp:857 +public: + ParmVarDeclChecker(FunctionDecl *FD) { + Parms.insert(FD->param_begin(), FD->param_end()); ---------------- This should accept `FD` as a const pointer. ================ Comment at: lib/Sema/SemaExpr.cpp:368 + + if (DiagnoseIfAttr *A = + checkArgIndependentDiagnoseIf(FD, DiagnoseIfWarnings)) { ---------------- `const DiagnoseIfAttr *`? ================ Comment at: lib/Sema/SemaExpr.cpp:388 + + for (DiagnoseIfAttr *W : DiagnoseIfWarnings) + emitDiagnoseIfDiagnostic(Loc, W); ---------------- `const auto *`? ================ Comment at: lib/Sema/SemaExpr.cpp:5186 + + for (DiagnoseIfAttr *Attr : Nonfatal) + S.emitDiagnoseIfDiagnostic(Fn->getLocStart(), Attr); ---------------- `const auto *`? ================ Comment at: lib/Sema/SemaOverload.cpp:827 destroyCandidates(); - ConversionSequenceAllocator.Reset(); - NumInlineSequences = 0; + // DiagnoseIfAttrs are just pointers, so we don't need to destroy them. + SlabAllocator.Reset(); ---------------- DiagnoseIfAttrs aren't the only thing allocated with the slab allocator though, right? Since this is being generalized, I wonder if asserting somewhere that the slab allocator only deals with pointers would make sense? ================ Comment at: lib/Sema/SemaOverload.cpp:6151 + bool MissingImplicitThis) { + auto EnableIfAttrs = getOrderedEnableIfAttrs(Function); + if (EnableIfAttrs.empty()) ---------------- Please spell out the type instead of using `auto`, since the type isn't spelled in the initialization. ================ Comment at: lib/Sema/SemaOverload.cpp:6178-6179 + SmallVectorImpl<DiagnoseIfAttr *> &Nonfatal) { + for (Attr *A : Function->attrs()) + if (auto *DIA = dyn_cast<DiagnoseIfAttr>(A)) + if (ArgDependent == DIA->getArgDependent()) { ---------------- Instead of doing this, you can use `specific_attrs<DiagnoseIfAttr>()` ================ Comment at: lib/Sema/SemaOverload.cpp:9009 +static bool isCandidateUnavailableDueToDiagnoseIf(const OverloadCandidate &OC) { + return OC.NumTriggeredDiagnoseIfs == 1 && + OC.DiagnoseIfInfo.get<DiagnoseIfAttr *>()->isError(); ---------------- Can you run into a situation with stacked diagnose_if errors, so that there's > 1? ================ Comment at: lib/Sema/SemaOverload.cpp:9101 + for (DiagnoseIfAttr *W : Best->getDiagnoseIfInfo()) { + assert(W->isWarning() && "Errors should've been caught earlier!"); ---------------- Can use `const auto *` here. ================ Comment at: lib/Sema/SemaOverload.cpp:12672 + + for (DiagnoseIfAttr *Attr : Nonfatal) + emitDiagnoseIfDiagnostic(MemE->getMemberLoc(), Attr); ---------------- Can use `const auto *` here. ================ Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:193 + S.Diag(A->getLocation(), diag::err_attr_cond_never_constant_expr) + << A->getSpelling(); + for (const PartialDiagnosticAt &P : Diags) ---------------- You shouldn't have to use `getSpelling()` here; I believe the diagnostics engine has an overload to properly handle `Attr *` objects. ================ Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:194 + << A->getSpelling(); + for (const PartialDiagnosticAt &P : Diags) + S.Diag(P.first, P.second); ---------------- `const auto &` https://reviews.llvm.org/D27424 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits