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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
