[PATCH] D146323: inline stmt attribute diagnosing in templates

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/test/Sema/attr-alwaysinline.cpp:48
+  if constexpr (D>0) {
+    // expected-warning at +6{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}}
+    // expected-note@#NO_DEP{{conflicting attribute is here}}
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > Why is this not included in the `3` for:  `// expected-warning at +4 3{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}}`
> > 
> > It's pretty unfortunate just how often this diagnostic triggers for the same line of code. This seems rather chatty, can you help me understand what's going on?
> I split them because they are diagnosing different 'things'.  See the associated 'note' on it.  
> 
> THIS warning is the one that happens while forming the primary template (the non-dependent call to `non_dependent`), and is later suppressed in the individual instantiations (which is the logic with the 'old' statement in `SemaStmtAttr.cpp`).  
> 
> The set on line 50 is for `baz`, which we emit 3x, because we're instantiating this function 3x.  Suppressing these isn't particularly possible without putting some sort of state in the AST of 'have we diagnosed this' as far as I can tell.
> 
> The variadic case below defeats our suppression entirely, which is why it diagnoses so often.  The non-dependent case can no longer be suppressed, because we don't have a good hold of which pre-instantiation call-exprs are associated with the post-instantiated call-exprs.
> 
> 
As far as the 'chattiness', I suspect using these attributes on a statement with multiple call expressions is likely pretty rare.  Using it on a dependent call is probably less likely.

That said, this attribute is a ergonomic nightmare if there are any call expressions in an argument list...  I'm hoping whoever uses this is aware of that!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146323/new/

https://reviews.llvm.org/D146323