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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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

See the comments inline.



================
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.
----------------
Could you leave a FIXME here to explore options around interactive fix selection?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:132
+      // choose the first one to apply.
+      const llvm::StringMap<Replacements> *ErrorFix = nullptr;
+      if (!Error.Message.Fix.empty())
----------------
`ErrorFix` brings more questions than it answers. Maybe `SelectedFix`, `ChosenFix`, or just `Fix`?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:133
+      const llvm::StringMap<Replacements> *ErrorFix = nullptr;
+      if (!Error.Message.Fix.empty())
+        ErrorFix = &Error.Message.Fix;
----------------
nit: I'd add braces here, since the `else` branch has them.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:144
+
+      if (ApplyFixes && ErrorFix) {
+        for (const auto &FileAndReplacements : *ErrorFix) {
----------------
The nesting level starts getting out of control here. I'd try to pull the loop into a function.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:187
       }
+      reportFix(Diag, Error.Message.Fix);
     }
----------------
Why `Error.Message.Fix`? Should this use `*SelectedFix` instead?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:262
+      for (const auto &Repl : FileAndReplacements.second) {
+        if (Repl.isApplicable()) {
+          SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
----------------
nit: Early "continue" would make sense here.

  if (!Repl.isApplicable())
    continue;
  ...


================
Comment at: clang/include/clang/Tooling/Core/Diagnostic.h:46
+
+  /// Fixes for this diagnostic.
+  llvm::StringMap<Replacements> Fix;
----------------
Some information from the original comment was lost here:

  /// Fixes to apply, grouped by file path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932