[analyzer] AnalyzerOptions: Remove 'fixits-as-remarks'

[Charusso]

Summary

The new way of checking fix-its is %check_analyzer_fixit.

[xazax.hun]

Maybe Kristof has some opinion whether we still need this :) He might be up to date whether CodeChecker is using this feature.

[NoQ]

Great, thanks!!

[NoQ]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

310

In D73729#1891015, @xazax.hun wrote:

Maybe Kristof has some opinion whether we still need this :) He might be up to date whether CodeChecker is using this feature.

Well, it clearly said "for testing purposes" :P

It should be much easier for CodeChecker to read fixits from plists.

[xazax.hun]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

310

Fair point :) LGTM!

[Szelethus]

Looks great! Thanks!

[Charusso]

[Achievement unlocked] 3 green marks.

Thanks!

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

ANALYZER_OPTION(bool, ShouldTrackConditions, "track-conditions",
                 "Whether to track conditions that are a control dependency of "
                 "an already tracked variable.",
                 true)
 
 ANALYZER_OPTION(bool, ShouldTrackConditionsDebug, "track-conditions-debug",
                 "Whether to place an event at each tracked condition.",
                 false)
 
-ANALYZER_OPTION(bool, ShouldEmitFixItHintsAsRemarks, "fixits-as-remarks",
-                "Emit fix-it hints as remarks for testing purposes",
-                false)
-
 ANALYZER_OPTION(bool, ShouldApplyFixIts, "apply-fixits",
                 "Apply the fix-it hints to the files",
                 false)
 
 //===----------------------------------------------------------------------===//
 // Unsigned analyzer options.
 //===----------------------------------------------------------------------===//
 

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

 
 namespace {
 class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer {
   DiagnosticsEngine &Diag;
   LangOptions LO;
 
   bool IncludePath = false;
   bool ShouldEmitAsError = false;
-  bool FixitsAsRemarks = false;
   bool ApplyFixIts = false;
 
 public:
   ClangDiagPathDiagConsumer(DiagnosticsEngine &Diag, LangOptions LO)
       : Diag(Diag), LO(LO) {}
   ~ClangDiagPathDiagConsumer() override {}
   StringRef getName() const override { return "ClangDiags"; }
 
   bool supportsLogicalOpControlFlow() const override { return true; }
   bool supportsCrossFileDiagnostics() const override { return true; }
 
   PathGenerationScheme getGenerationScheme() const override {
     return IncludePath ? Minimal : None;
   }
 
   void enablePaths() { IncludePath = true; }
   void enableWerror() { ShouldEmitAsError = true; }
-  void enableFixitsAsRemarks() { FixitsAsRemarks = true; }
   void enableApplyFixIts() { ApplyFixIts = true; }
 
   void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
                             FilesMade *filesMade) override {
     unsigned WarnID =
         ShouldEmitAsError
             ? Diag.getCustomDiagID(DiagnosticsEngine::Error, "%0")
             : Diag.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
     unsigned NoteID = Diag.getCustomDiagID(DiagnosticsEngine::Note, "%0");
-    unsigned RemarkID = Diag.getCustomDiagID(DiagnosticsEngine::Remark, "%0");
     SourceManager &SM = Diag.getSourceManager();
 
     Replacements Repls;
     auto reportPiece = [&](unsigned ID, FullSourceLoc Loc, StringRef String,
                            ArrayRef<SourceRange> Ranges,
                            ArrayRef<FixItHint> Fixits) {
-      if (!FixitsAsRemarks && !ApplyFixIts) {
+      if (!ApplyFixIts) {
         Diag.Report(Loc, ID) << String << Ranges << Fixits;
         return;
       }
 
       Diag.Report(Loc, ID) << String << Ranges;
-      if (FixitsAsRemarks) {
-        for (const FixItHint &Hint : Fixits) {
-          llvm::SmallString<128> Str;
-          llvm::raw_svector_ostream OS(Str);
-          // FIXME: Add support for InsertFromRange and
-          // BeforePreviousInsertion.
-          assert(!Hint.InsertFromRange.isValid() && "Not implemented yet!");
-          assert(!Hint.BeforePreviousInsertions && "Not implemented yet!");
-          OS << SM.getSpellingColumnNumber(Hint.RemoveRange.getBegin()) << "-"
-             << SM.getSpellingColumnNumber(Hint.RemoveRange.getEnd()) << ": '"
-             << Hint.CodeToInsert << "'";
-          Diag.Report(Loc, RemarkID) << OS.str();
-        }
-      }
-
-      if (ApplyFixIts) {
-        for (const FixItHint &Hint : Fixits) {
-          Replacement Repl(SM, Hint.RemoveRange, Hint.CodeToInsert);
 
-          if (llvm::Error Err = Repls.add(Repl)) {
-            llvm::errs() << "Error applying replacement " << Repl.toString()
-                         << ": " << Err << "\n";
-          }
-        }
-      }
     };
 
     for (std::vector<const PathDiagnostic *>::iterator I = Diags.begin(),
          E = Diags.end();
          I != E; ++I) {
       const PathDiagnostic *PD = *I;
       reportPiece(WarnID, PD->getLocation().asLocation(),
                   PD->getShortDescription(), PD->path.back()->getRanges(),
if (Opts->AnalysisDiagOpt != PD_NONE) {
       // Create the PathDiagnosticConsumer.
       ClangDiagPathDiagConsumer *clangDiags =
           new ClangDiagPathDiagConsumer(PP.getDiagnostics(), PP.getLangOpts());
       PathConsumers.push_back(clangDiags);
 
       if (Opts->AnalyzerWerror)
         clangDiags->enableWerror();
 
-      if (Opts->ShouldEmitFixItHintsAsRemarks)
-        clangDiags->enableFixitsAsRemarks();
-
       if (Opts->ShouldApplyFixIts)
         clangDiags->enableApplyFixIts();
 
       if (Opts->AnalysisDiagOpt == PD_TEXT) {
         clangDiags->enablePaths();
 
       } else if (!OutDir.empty()) {
         switch (Opts->AnalysisDiagOpt) {

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: debug.AnalysisOrder:RegionChanges = false
 // CHECK-NEXT: display-ctu-progress = false
 // CHECK-NEXT: eagerly-assume = true
 // CHECK-NEXT: elide-constructors = true
 // CHECK-NEXT: expand-macros = false
 // CHECK-NEXT: experimental-enable-naive-ctu-analysis = false
 // CHECK-NEXT: exploration_strategy = unexplored_first_queue
 // CHECK-NEXT: faux-bodies = true
-// CHECK-NEXT: fixits-as-remarks = false
 // CHECK-NEXT: graph-trim-interval = 1000
 // CHECK-NEXT: inline-lambdas = true
 // CHECK-NEXT: ipa = dynamic-bifurcate
 // CHECK-NEXT: ipa-always-inline-size = 3
 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-symbol-complexity = 35
 // CHECK-NEXT: max-times-inline-large = 32

 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 98
+// CHECK-NEXT: num-entries = 97

clang/test/Analysis/dead-stores.c

-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -Wunused-variable -fblocks -Wno-unreachable-code \
 // RUN:   -analyzer-checker=core,deadcode.DeadStores \
 // RUN:   -analyzer-config deadcode.DeadStores:ShowFixIts=true \
-// RUN:   -analyzer-config fixits-as-remarks=true \
 // RUN:   -analyzer-config \
 // RUN:       deadcode.DeadStores:WarnForDeadNestedAssignments=false \
-// RUN:   -verify=non-nested %s
+// RUN:   -verify=non-nested
 
-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -Wunused-variable -fblocks -Wno-unreachable-code \
 // RUN:   -analyzer-checker=core,deadcode.DeadStores \
 // RUN:   -analyzer-config deadcode.DeadStores:ShowFixIts=true \
-// RUN:   -analyzer-config fixits-as-remarks=true \
-// RUN:   -verify=non-nested,nested %s
+// RUN:   -verify=non-nested,nested
 
 void f1() {
   int k, y; // non-nested-warning {{unused variable 'k'}}
             // non-nested-warning@-1 {{unused variable 'y'}}
   int abc = 1;
   long idx = abc + 3 * 5; // non-nested-warning {{never read}}
                           // non-nested-warning@-1 {{unused variable 'idx'}}
-                          // non-nested-remark@-2 {{11-24: ''}}
+  // CHECK-FIXES:      int abc = 1;
+  // CHECK-FIXES-NEXT: long idx;
 }
 
 void f2(void *b) {
   char *c = (char *)b; // no-warning
   char *d = b + 1;     // non-nested-warning {{never read}}
                        // non-nested-warning@-1 {{unused variable 'd'}}
-                       // non-nested-remark@-2 {{10-17: ''}}
+  // CHECK-FIXES:      char *c = (char *)b;
+  // CHECK-FIXES-NEXT: char *d;
+
   printf("%s", c);
   // non-nested-warning@-1 {{implicitly declaring library function 'printf' with type 'int (const char *, ...)'}}
   // non-nested-note@-2 {{include the header <stdio.h> or explicitly provide a declaration for 'printf'}}
 }
 
 int f();
 void f3() {
   int r;
if (k)
     f1();
   k = 2; // non-nested-warning {{never read}}
 }
 
 void f5() {
   int x = 4;   // no-warning
   int *p = &x; // non-nested-warning {{never read}}
                // non-nested-warning@-1 {{unused variable 'p'}}
-               // non-nested-remark@-2 {{9-13: ''}}
+  // CHECK-FIXES:      int x = 4;
+  // CHECK-FIXES-NEXT: int *p;
 }
 
 int f6() {
   int x = 4;
   ++x; // no-warning
   return 1;
 }
 
else
       f23_aux("I shouldn't log.  Wait.. d'oh!\n");
   }();
 }
 
 void f23_pos(int argc, char **argv) {
   int shouldLog = (argc > 1);
   // non-nested-warning@-1 {{Value stored to 'shouldLog' during its initialization is never read}}
   // non-nested-warning@-2 {{unused variable 'shouldLog'}}
-  // non-nested-remark@-3 {{16-28: ''}}
+  // CHECK-FIXES:      void f23_pos(int argc, char **argv) {
+  // CHECK-FIXES-NEXT:   int shouldLog;
   ^{
     f23_aux("I did too use it!\n");
   }();
 }
 
 void f24_A(int y) {
   // FIXME: One day this should be reported as dead since 'z = x + y' is dead.
   int x = (y > 2); // no-warning
   ^{
     int z = x + y;
     // non-nested-warning@-1 {{Value stored to 'z' during its initialization is never read}}
     // non-nested-warning@-2 {{unused variable 'z'}}
-    // non-nested-remark@-3 {{10-17: ''}}
+    // CHECK-FIXES:      void f24_A(int y) {
+    // CHECK-FIXES-NEXT:   //
+    // CHECK-FIXES-NEXT:   int x = (y > 2);
+    // CHECK-FIXES-NEXT:   ^{
+    // CHECK-FIXES-NEXT:     int z;
   }();
 }
 
 void f24_B(int y) {
   // FIXME: One day this should be reported as dead since 'x' is just overwritten.
   __block int x = (y > 2); // no-warning
   ^{
     // FIXME: This should eventually be a dead store since it is never read either.

clang/test/Analysis/virtualcall-fixit.cpp

-// RUN: %check_analyzer_fixit %s %t \
-// RUN:   -analyzer-checker=core,optin.cplusplus.VirtualCall \
-// RUN:   -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true
-
-struct S {
-  virtual void foo();
-  S() {
-    foo();
-    // expected-warning@-1 {{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
-    // CHECK-FIXES: S::foo();
-  }
-  ~S();
-};

clang/test/Analysis/virtualcall-fixits.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
 // RUN:     -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \
 // RUN:     %s 2>&1 | FileCheck -check-prefix=TEXT %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:     -analyzer-checker=core,optin.cplusplus.VirtualCall \
 // RUN:     -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \
-// RUN:     -analyzer-config fixits-as-remarks=true \
-// RUN:     -analyzer-output=plist -o %t.plist -verify %s
+// RUN:     -analyzer-output=plist -o %t.plist
 // RUN: cat %t.plist | FileCheck -check-prefix=PLIST %s
 
 struct S {
   virtual void foo();
   S() {
     foo();
     // expected-warning@-1{{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
-    // expected-remark@-2{{5-5: 'S::'}}
+    // CHECK-FIXES:      S() {
+    // CHECK-FIXES-NEXT:   S::foo();
+    // CHECK-FIXES-NEXT: }
   }
   ~S();
 };
 
 // TEXT: warning: Call to virtual method 'S::foo' during construction
 // TEXT-SAME: bypasses virtual dispatch
 // TEXT-NEXT: foo();
 // TEXT-NEXT: ^~~~~
 // TEXT-NEXT: S::
 // TEXT-NEXT: 1 warning generated.
 
 // PLIST:  <key>fixits</key>
 // PLIST-NEXT:  <array>
 // PLIST-NEXT:   <dict>
 // PLIST-NEXT:    <key>remove_range</key>
 // PLIST-NEXT:    <array>
 // PLIST-NEXT:     <dict>
-// PLIST-NEXT:      <key>line</key><integer>13</integer>
+// PLIST-NEXT:      <key>line</key><integer>14</integer>
 // PLIST-NEXT:      <key>col</key><integer>5</integer>
 // PLIST-NEXT:      <key>file</key><integer>0</integer>
 // PLIST-NEXT:     </dict>
 // PLIST-NEXT:     <dict>
-// PLIST-NEXT:      <key>line</key><integer>13</integer>
+// PLIST-NEXT:      <key>line</key><integer>14</integer>
 // PLIST-NEXT:      <key>col</key><integer>4</integer>
 // PLIST-NEXT:      <key>file</key><integer>0</integer>
 // PLIST-NEXT:     </dict>
 // PLIST-NEXT:    </array>
 // PLIST-NEXT:    <key>insert_string</key><string>S::</string>
 // PLIST-NEXT:   </dict>
 // PLIST-NEXT:  </array>