[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.
Herald added subscribers: martong, steakhal.

A few nits.



================
Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:93
+
+  bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false,
+       ApplyFixIts = false;
----------------
One variable per definition would result in a more readable code, IMO.


================
Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:112
   void enableFixitsAsRemarks() { FixitsAsRemarks = true; }
+  void enableFixItApplication() { ApplyFixIts = true; }
 
----------------
nit: I'd suggest naming the method closer to the name of the corresponding field, e.g. `enableApplyFixIts`. Why isn't this `setApplyFixIts(bool)` btw?


================
Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:154-155
+          if (llvm::Error Err = Repls.add(Repl)) {
+            llvm::errs() << "An error occured during fix-it replacement:\n'"
+                         << Repl.toString() << "'\nThe error message: '" << Err
+                         << "'\n";
----------------
Cosmetics: I'd suggest to make the message less verbose. Specifically, to change

  An error occured during fix-it replacement:
     <replacement>
  The error message: <error message>

to

  Error applying replacement <replacement>: <error message>


================
Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:198
+    if (!applyAllReplacements(Repls, Rewrite)) {
+      llvm::errs() << "An error occured during applying fix-it replacements.\n";
+    }
----------------
s/fix-it replacements/fix-it/


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

https://reviews.llvm.org/D69746