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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked 15 inline comments as done.
Szelethus added a comment.

Thanks, @martong!



================
Comment at: clang/include/clang/StaticAnalyzer/Core/Analyses.def:61
+
+ANALYSIS_DIAGNOSTICS(TEXT_MINIMAL, "text-minimal",
+                     "Emits minimal diagnostics to stderr, stating only the "
----------------
martong wrote:
> Just out of curiosity: is there a way to know which one is the default?
Not from this file, unfortunately. You need to go to `AnalyzerOptions.h`. It would be great if we were able to list the possible options as well as the default value (similarly to `-analyzer-config`s), but I don't have plant to do it myself anytime soon.


================
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
----------------
martong wrote:
> 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`?
`AnalyzerOptions.h`.


================
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
----------------
martong wrote:
> Man, I understand you pain now, this is really muddled, do you plan to sort this out in an upcoming patch?
Allow me to indulge you with the relieving fix found in D76510 :)


================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:148
+  // TODO: Emit an error here.
+  if (OutputDir.empty())
+    return;
----------------
martong wrote:
> 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 ?
> 
  if (OutputDir.empty())
    return;

  createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
  C.push_back(new HTMLDiagnostics(AnalyzerOpts, OutputDir, PP, true));

Otherwise this would be a functional //and// breaking change. Mind that in this patch, HTML is still our default output type. If I were to move the text diagnostics below this line, we would no longer emit any diagnostics to stderr. I actually had the same idea, got ~670 breaking test :^)

  if (OutputDir.empty()) {
    createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
    return;
  }

  C.push_back(new HTMLDiagnostics(AnalyzerOpts, OutputDir, PP, true));

With this code, we would no longer emit to stderr, only to HTML, should an output directory be specified.


================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:148
+  // TODO: Emit an error here.
+  if (OutputDir.empty())
+    return;
----------------
Szelethus wrote:
> martong wrote:
> > 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 ?
> > 
>   if (OutputDir.empty())
>     return;
> 
>   createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
>   C.push_back(new HTMLDiagnostics(AnalyzerOpts, OutputDir, PP, true));
> 
> Otherwise this would be a functional //and// breaking change. Mind that in this patch, HTML is still our default output type. If I were to move the text diagnostics below this line, we would no longer emit any diagnostics to stderr. I actually had the same idea, got ~670 breaking test :^)
> 
>   if (OutputDir.empty()) {
>     createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
>     return;
>   }
> 
>   C.push_back(new HTMLDiagnostics(AnalyzerOpts, OutputDir, PP, true));
> 
> With this code, we would no longer emit to stderr, only to HTML, should an output directory be specified.
> Would be an assert better here?

No, the point of the patch is to handle user errors (such as setting the output type to html but forgetting to supply the output location) in these factory functions. An assert would've been appropriate if I left `AnalysisConsumer::DigestAnalyzerOptions` untouched, as previously, that is where we checked such inconsistencies.


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

* Moved the initialization of the boolean fields into the constructor (these were previously done with setters in `AnalysisConsumer::DigestAnalyzerOptions`)
* Renamed the `DiagnosticsEngine &` field from `Diag` to `DiagEng` (because this entire file is about diagnostics, duh)
* Instead of `IncludePath` being set according to the value of `Opts->AnalysisDiagOpt == PD_TEXT`, it depends on whether `createTextMinimalPathDiagnosticConsumer` or `createTextPathDiagnosticConsumer` is called. But as to which of these is called, of course, depends on `Opts->AnalysisDiagOpt == PD_TEXT`.

This is such an essential part of the analyzer, almost every test file implicitly checks this code as well, so I don't think there's much to worry about here :)


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


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


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

https://reviews.llvm.org/D76509