[PATCH] D28889: Change where we handle arg-dependent diagnose_if attributes

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Another "fun" testcase:

  struct S {
    void operator++(int n) _diagnose_if(n, "wat", "warning");
  };
  void f(S s) {
    s++; // no warning
    s.operator++(1); // warning
  }



================
Comment at: include/clang/Sema/Sema.h:2638
 
-  /// Check the diagnose_if attributes on the given function. Returns the
-  /// first succesful fatal attribute, or null if calling Function(Args) isn't
-  /// an error.
+  /// Emit diagnostics for the diagnose_if attributes on Function, ignoring any
+  /// non-ArgDependent DiagnoseIfAttrs.
----------------
Can you add a comment somewhere about here explaining why these functions are split? Something like "Argument-dependent diagnose_if attributes are checked when the function is used as a direct callee of a function call." here, and "Argument-independent diagnose_if attributes are checked on every use of the function." below.


================
Comment at: include/clang/Sema/Sema.h:9925
                  SourceLocation Loc, SourceRange Range,
-                 VariadicCallType CallType);
+                 VariadicCallType CallType, const Expr *ThisArg);
 
----------------
We have a loose convention that function parameter argument order matches source order, which would suggest that `ThisArg` should precede `Args` here.


================
Comment at: lib/Sema/SemaExprCXX.cpp:6717
+
+  checkDiagnoseIfAttrsOnCall(Method, CE);
   return CE;
----------------
Can you call  `CheckFunctionCall` instead here, and remove `checkDiagnoseIfAttrsOnCall`? It looks like the only reason we don't already call that is because none of its checks could ever fire for a call to a conversion function before, and that's no longer true.


================
Comment at: lib/Sema/SemaOverload.cpp:12035
 
+      checkDiagnoseIfAttrsOnCall(FnDecl, TheCall);
       return MaybeBindToTemporary(TheCall);
----------------
Likewise call `CheckFunctionCall` here.


================
Comment at: lib/Sema/SemaOverload.cpp:12488
 
+        checkDiagnoseIfAttrsOnCall(Method, TheCall);
         return MaybeBindToTemporary(TheCall);
----------------
... and here.


================
Comment at: lib/Sema/SemaOverload.cpp:13228
 
+  checkDiagnoseIfAttrsOnCall(Method, TheCall);
   return MaybeBindToTemporary(TheCall);
----------------
... and here.


================
Comment at: test/SemaCXX/diagnose_if.cpp:614
+    // FIXME: This should emit diagnostics. It seems that our constexpr
+    // evaluator isn't able to evaluate `adl::Foo(1)` to a constexpr, though.
+    // I'm assuming this is because we assign it to a temporary.
----------------
`constexpr` is an adjective; "to a constant" might make more sense.


================
Comment at: test/SemaCXX/diagnose_if.cpp:615
+    // evaluator isn't able to evaluate `adl::Foo(1)` to a constexpr, though.
+    // I'm assuming this is because we assign it to a temporary.
+    for (void *p : adl::Foo(1)) {}
----------------
The range-based for is desugared to

```
auto &&__range = adl::Foo(1);
auto __begin = begin(__range);
auto __end = end(__range);
// ...
```

so the argument in the call to `begin` is not considered constant.


https://reviews.llvm.org/D28889