[PATCH] D76509: [analyzer][NFC] Move the text output type to its own file, move code to PathDiagnosticConsumer creator functions

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/Analyses.def:17
 
-ANALYSIS_STORE(RegionStore, "region", "Use region-based analyzer store", CreateRegionStoreManager)
+ANALYSIS_STORE(RegionStore, "region", "Use region-based analyzer store",
+               CreateRegionStoreManager)
----------------
Yay! I am a big fun of the 80 col limit! :)


================
Comment at: clang/include/clang/StaticAnalyzer/Core/Analyses.def:61
+
+ANALYSIS_DIAGNOSTICS(TEXT_MINIMAL, "text-minimal",
+                     "Emits minimal diagnostics to stderr, stating only the "
----------------
Just out of curiosity: is there a way to know which one is the default?


================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:140
+
+  // FIXME: HTML is currently our default output type, but if the output
+  // directory isn't specified, it acts like if it was in the minimal text
----------------
Actually, now I wonder where should we document the default output type ... I am pretty sure it should be done somewhere where it is obvious, maybe in `Analyses.def`?


================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:142
+  // directory isn't specified, it acts like if it was in the minimal text
+  // output mode. This doesn't make much sense, we should have the minimal text
+  // as our default. In the case of backward compatibility concerns, this could
----------------
Man, I understand you pain now, this is really muddled, do you plan to sort this out in an upcoming patch?


================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:148
+  // TODO: Emit an error here.
+  if (OutputDir.empty())
+    return;
----------------
Would be an `assert` better here?


================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:148
+  // TODO: Emit an error here.
+  if (OutputDir.empty())
+    return;
----------------
martong wrote:
> Would be an `assert` better here?
```
 // if the output
  // directory isn't specified, it acts like if it was in the minimal text
  // output mode.
```
So, why don't we do this check **before** calling createTextMinimalPathDiagnosticConsumer ?



================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:1
+//===--- TextDiagnostics.cpp - Text Diagnostics for Paths -------*- C++ -*-===//
+//
----------------
Is this just a blind copy (and rename) from `AnalysisConsumer`, or should I take a deeper look into anything here?


================
Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:156
 #include "clang/StaticAnalyzer/Core/Analyses.def"
-        }
-      }
+    default:
+      llvm_unreachable("Unkown analyzer output type!");
----------------
Some build bots will not compile if you handle all cases in the swtich and you still have a `default`, beware.


================
Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:348
   void reportAnalyzerProgress(StringRef S);
-};
+}; // namespace
 } // end anonymous namespace
----------------
Either the `;` is redundant or this is not the end of a namespace.


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

https://reviews.llvm.org/D76509