[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.
Herald added a subscriber: carlosgalvezp.
Herald added a project: All.

Someone else ultimately fixed this (slightly differently) last month:
https://reviews.llvm.org/D146875


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

Thanks for your reply and sorry about my very sluggish reply!
I am looking into updating the docs as you suggested, and that got me looking 
at this doc page 
.
 Interestingly, that doc page's version of the add_new_check.py template 
doesn't have the `Note` diagnostic. So that got me digging into the history.
It seems like 2 commits are at odds with each other about what the right thing 
to do is with `FixItHint`s w.r.t. `Note` vs `Warning` diagnostics.

1. This commit 

 says that "fix descriptions and fixes are emitted via diagnostic::Note (rather 
than attaching the main warning diagnostic)."  - And note how that commit 
changes add_new_check.py - adding the `Note` diagnostic!
2. This commit 
 seems to 
suggest that Note diagnostics should //not// be used to provide the "standard" 
fix (and generally enforces that by `--fix-notes` being off by default)

It looks like nearly all existing check implementations disagree with the 
former, and agree with the latter.
Those two commits appear to be at odds with each other on the semantics of 
`Note` diagnostics - any opinions on how to reconcile that apparent 
disagreement?

My (limited) perspective is:
Since `--fix-notes` is off by default, it seems like `FixIt`s on `Note` 
diagnostics are pretty well enforced to be used for secondary/uncertain 
suggestions, while `FixIt`s on `Warning` diagnostics are for the primary 
fix(es). 
That seems to rule out accepting the guidance of first commit w.r.t. attaching 
fixes to `Note` diagnostics given the `--fix-notes` flag imposes that such 
note-fixes are implicitly secondary to warning-fixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@mattbeardsley Thank you for the explanation. (I'd wager some of your 
observations could even be added to some developer docs that's versioned in the 
repo. ) I'm not opposed to the changes at all. And indeed, it looks like 
`Note`s are really an advanced feature. I'm incredibly biased because I just 
recently finished the development of a checker in which I extensively use notes 
(in some test cases, there were like 8 or 9 notes attached to a warning, albeit 
all without a `FixIt` attached).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

Ah, and to your direct question:

> BTW, the test wasn't checking for the note's message at all?"

Right, the generated test file does not ever do `// CHECK-MESSAGES: :Row:Col: 
note: insert 'awesome'` (nor does the LIT setup here enforce that notes need to 
be checked - it enforces that all warning and error diagnostics must be 
checked, but it looks like it leaves out note diagnostics)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley added a comment.

In D109210#2981912 , @whisperity 
wrote:

> Uuuh.. I get the sentiment, but this change breaks the very essence of the 
> joke that the default generated code wants to pass on. It also does not 
> showcase that we can emit notes, not just warnings.
>
> How about `function %0 is insufficiently awesome, mark it as 'awesome_'!` in 
> the warning text? (Note how the fix and the note's text diverged already.)
>
> And so we should come up with a joke for the Note tag.
>
> BTW, the test wasn't checking for the note's message at all?

I'm certainly not trying to be a buzzkill on the joke :)
Yes, I did notice that the printed diagnostic text changed from its original, 
but I'll add some background investigation that I should have written in the 
original review text, sorry about laying out quite an incomplete thought 
process. 
At least, I'm hoping I can show there's some extent of due diligence 
behind-the-scenes, and that I'm not just trying to change this willy-nilly!

I read/grepped/etc through several (but certainly not all) existing checks to 
try to identify what the "canonical" diagnostic-and-fix pattern looks like. 
I was thinking that a decent goal would be that, if someone takes the files 
generated by add_new_check.py as their starting point and runs with it, they'll 
end up with check behavior that's reasonably consistent with a "typical" 
existing check.

Here are my observations w.r.t "note" diagnostic usage and diagnostic message 
wording:

1. < ~15% of existing checks use "note" diagnostics

  $ cd clang-tools-extra/clang-tidy
  
  # 40 checks use notes
  $ find . -name '*Check.cpp' | xargs grep -nl 'DiagnosticIDs::Note' | wc -l
  40
  
  # out of 265 calling diag directly
  # (there are 287 total *Check.cpp - most of the remaining 22 are 
android/Cloexec*.cpp which call "CloexecCheck::replaceFunc", which also doesn't 
use DiagnosticIDs::Note)
  $ find . -name '*Check.cpp' | xargs grep -nl 'diag(' | wc -l
  265

2. Out of several messages that I skimmed through arbitrarily (subject to 
potentially being a misrepresentative sample, but hopefully not), the general 
pattern seems to be to state the problem, but not the fix in the handwritten 
diagnostic message text. Here are some examples from the readability module:
  - `readability-redundant-string-init` warns "redundant string 
initialization", but doesn't state "remove the initialization"
  - `readability-redundant-string-cstr` warns "redundant call to %0", but 
doesn't state "remove the call"
  - `readability-named-parameter` warns "all parameters should be named in a 
function", but doesn't state "insert the commented parameter name"
  - `readability-implicit-bool-conversion` warns "implicit conversion bool -> 
%0", but doesn't state the various insertions and removals it's going to do
  - `readability-braces-around-statements` warns "statement should be inside 
braces", but doesn't state "insert statements"

The typical pattern appeared to be to state the issue, then let the FixItHint 
display the suggested conversion, without writing text describing that 
conversion explicitly.
So I aimed to adjust the add_new_check.py script so that it would produce an 
example that's consistent with those observed best practices from the live 
code, so that a new check based on add_new_check.py would look reasonably 
similar to surrounding checks. 
My interpretation of that was to state the issue ("insufficiently awesome"), 
but not describe the insertion - leaving that to the FixItHint::CreateInsertion 
to emit as it sees fit.

Then, since a relatively small percentage of checks use Note diagnostics, my 
take on it was that including the Note example would lead a newcomer to 
unnecessarily start adding Note diagnostics. 
This is anecdotal evidence obviously, but after a couple years spent (on & off) 
writing about 40 clang-tidy checks in my organization, I only just learned 
yesterday from a git bisect that I'd been incorrectly using note diagnostics! 
("why did all my tests start failing after pulling from upstream!"). 
And I'd been using Note diagnostics that way all along because my impression 
from the add_new_check.py example was that streaming the FixItHints into Note 
diagnostics was the right thing to do for the "standard" fix in a clang-tidy 
check.

Last - given relatively few (~15%) checks actually use Note diagnostics at all, 
it didn't seem like Note diagnostics had "earned" their place in the generated 
example code, and could be left as a slightly more "advanced maneuver" instead
Slightly more (42, still about 15%) Check.cpp files (using similar find/grep as 
above methodology) use "Lexer::getSourceText," but that's not part of the 
add_new_check.py example. 
So I guess I'm left wondering - what drives Note diagnostics to be a crucial 
piece to keep as a demo in add_new_check.py, as opposed to showcasing a 
different maneuver? Is there something else that would be 

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Uuuh.. I get the sentiment, but this change breaks the very essence of the joke 
that the default generated code wants to pass on. It also does not showcase 
that we can emit notes, not just warnings.

How about `function %0 is insufficiently awesome, mark it as 'awesome_'!` in 
the warning text? (Note how the fix and the note's text diverged already.)

And so we should come up with a joke for the Note tag.

BTW, the test wasn't checking for the note's message at all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

[Matt Beardsley via Phabricator via cfe-commits]

mattbeardsley created this revision.
mattbeardsley added reviewers: njames93, aaron.ballman.
Herald added a subscriber: xazax.hun.
mattbeardsley requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Per this commit, "Note" diagnostics should not be used to provide the default
fix behavior:
https://reviews.llvm.org/rGabbe9e227ed31e5dde9bb7567bb9f0dd047689c6

With the default clang-tidy behavior being to not "--fix-notes" in the above
commit, the add_new_check.py script would generate a check that does not pass
its own generated test (before this change)

Because the generated example clang-tidy check has a clear default FixItHint,
based on the above commit, the FixItHint would be better suited by being sent to
the warning diag(...), as opposed to keeping the separate note diag(...) and
passing "--fix-notes" in the generated test


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109210

Files:
  clang-tools-extra/clang-tidy/add_new_check.py


Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -144,8 +144,7 @@
   if (!MatchedDecl->getIdentifier() || 
MatchedDecl->getName().startswith("awesome_"))
 return;
   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
-  << MatchedDecl;
-  diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
+  << MatchedDecl
   << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
 }
 


Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -144,8 +144,7 @@
   if (!MatchedDecl->getIdentifier() || MatchedDecl->getName().startswith("awesome_"))
 return;
   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
-  << MatchedDecl;
-  diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
+  << MatchedDecl
   << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits