[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D59932#1453565 <https://reviews.llvm.org/D59932#1453565>, @alexfh wrote:

> This looks like a more promising direction. Thanks for the readiness to experiment with this.
>
> See the comments inline.


Thanks for the comments. Now all existing tests are passed, the patch is in a good shape for review.

There is one missing point -- we don't test the fix-description notes in the lit tests, the current test mechanism (CHECK-MESSAGE, CHECK-NOTES) doesn't handle it well, we need to feature it out. I think this can be done in a separate patch.



================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:130
+
+      // If we have alternative fixes (attached via diagnostic::Notes), we just
+      // choose the first one to apply.
----------------
alexfh wrote:
> Could you leave a FIXME here to explore options around interactive fix selection?
Done. I moved this code to `Diagnostic::getChosenFix()` as we have a few places using it.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:144
+
+      if (ApplyFixes && ErrorFix) {
+        for (const auto &FileAndReplacements : *ErrorFix) {
----------------
alexfh wrote:
> The nesting level starts getting out of control here. I'd try to pull the loop into a function.
Agree, but making such refactoring change in this patch will add noise to the diff, I tend to make the change in this patch as mini as possible.

I think this can be improved in a follow-up change. 


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:187
       }
+      reportFix(Diag, Error.Message.Fix);
     }
----------------
alexfh wrote:
> Why `Error.Message.Fix`? Should this use `*SelectedFix` instead?
I think we still want to report all the alternative fixes to clients to align with clang's behavior. We only use the `SelectedFix` when applying the fix. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932