[PATCH] D84924: [clang-tidy][WIP] Added command line option `fix-notes`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D84924#2421116 <https://reviews.llvm.org/D84924#2421116>, @njames93 wrote:

> Fix documentation.
>
> Ping??

Ah, sorry, this fell to the bottom of my review list because the title still says [WIP] and so I thought there was more work being done on it. You may want to remove that bit from the title.

In D84924#2184132 <https://reviews.llvm.org/D84924#2184132>, @njames93 wrote:

> This is very much a work in progress
> Another direction I was thinking was only apply the fixes found in notes if there is exactly one fix attached to the notes in a diagnostic, instead of just applying the first one we find

I was wondering the same thing here myself. If there's exactly one fix, then it's unambiguous as to what behavior you get. One (minor) concern I have about the current approach is with nondeterminism in diagnostic ordering where different runs may pick different fixes for the same code. I don't *think* we have any instances of this in Clang or clang-tidy, but users can add their own plugins (for instance to the clang static analyzer) that may introduce some small risk there. Do you have a reason why you picked one approach over the other?



================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.h:92
 void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
-                  ClangTidyContext &Context, bool Fix,
+                  ClangTidyContext &Context, bool Fix, bool AnyFix,
                   unsigned &WarningsAsErrorsCount,
----------------
I think you should add some comments explaining the difference between `Fix` and `AnyFix` because it's not super clear. Alternatively, if these are strongly related, should this be an enumeration rather than two boolean parameters? e.g., `enum FixitBehavior { NoFixes, ApplyNonNoteFixes, ApplyAllFixes };` or some such (note, this suggestion may or may not make sense, I'm still reviewing).


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp:1
-// RUN: %check_clang_tidy %s misc-definitions-in-headers %t
+// RUN: %check_clang_tidy %s misc-definitions-in-headers %t -- -fix-notes
 
----------------
Hmm, should that be `--fix-notes`? (Note the number of dashes, which differs from the documentation)


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp:1
-// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t
+// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t -- -fix-notes
 void foo(int a) {
----------------
Same question here as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84924