[PATCH] D27424: Add the diagnose_if attribute to clang.

[Aaron Ballman via Phabricator via cfe-commits]

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