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