[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfd4b3f123d6e: [analyzer] NFC: Separate 
PathDiagnosticConsumer options from AnalyzerOptions. (authored by dergachev.a).
Herald added a subscriber: steakhal.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67420

Files:
  clang/include/clang/Analysis/PathDiagnostic.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -150,7 +150,7 @@
   break;
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)\
   case PD_##NAME:  \
-CREATEFN(*Opts.get(), PathConsumers, OutDir, PP, CTU); \
+CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
 #include "clang/StaticAnalyzer/Core/Analyses.def"
 default:
Index: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
@@ -34,20 +34,17 @@
 /// type to the standard error, or to to compliment many others. Emits detailed
 /// diagnostics in textual format for the 'text' output type.
 class TextDiagnostics : public PathDiagnosticConsumer {
+  PathDiagnosticConsumerOptions DiagOpts;
   DiagnosticsEngine 
   const LangOptions 
-  const bool IncludePath = false;
-  const bool ShouldEmitAsError = false;
-  const bool ApplyFixIts = false;
-  const bool ShouldDisplayCheckerName = false;
+  bool ShouldDisplayPathNotes;
 
 public:
-  TextDiagnostics(DiagnosticsEngine , const LangOptions ,
-  bool ShouldIncludePath, const AnalyzerOptions )
-  : DiagEng(DiagEng), LO(LO), IncludePath(ShouldIncludePath),
-ShouldEmitAsError(AnOpts.AnalyzerWerror),
-ApplyFixIts(AnOpts.ShouldApplyFixIts),
-ShouldDisplayCheckerName(AnOpts.ShouldDisplayCheckerNameForText) {}
+  TextDiagnostics(PathDiagnosticConsumerOptions DiagOpts,
+  DiagnosticsEngine , const LangOptions ,
+  bool ShouldDisplayPathNotes)
+  : DiagOpts(std::move(DiagOpts)), DiagEng(DiagEng), LO(LO),
+ShouldDisplayPathNotes(ShouldDisplayPathNotes) {}
   ~TextDiagnostics() override {}
 
   StringRef getName() const override { return "TextDiagnostics"; }
@@ -56,13 +53,13 @@
   bool supportsCrossFileDiagnostics() const override { return true; }
 
   PathGenerationScheme getGenerationScheme() const override {
-return IncludePath ? Minimal : None;
+return ShouldDisplayPathNotes ? Minimal : None;
   }
 
   void FlushDiagnosticsImpl(std::vector ,
 FilesMade *filesMade) override {
 unsigned WarnID =
-ShouldEmitAsError
+DiagOpts.ShouldDisplayWarningsAsErrors
 ? DiagEng.getCustomDiagID(DiagnosticsEngine::Error, "%0")
 : DiagEng.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
 unsigned NoteID = DiagEng.getCustomDiagID(DiagnosticsEngine::Note, "%0");
@@ -72,7 +69,7 @@
 auto reportPiece = [&](unsigned ID, FullSourceLoc Loc, StringRef String,
ArrayRef Ranges,
ArrayRef Fixits) {
-  if (!ApplyFixIts) {
+  if (!DiagOpts.ShouldApplyFixIts) {
 DiagEng.Report(Loc, ID) << String << Ranges << Fixits;
 return;
   }
@@ -92,9 +89,10 @@
  E = Diags.end();
  I != E; ++I) {
   const PathDiagnostic *PD = *I;
-  std::string WarningMsg =
-  (ShouldDisplayCheckerName ? " [" + PD->getCheckerName() + "]" : "")
-  .str();
+  std::string WarningMsg = (DiagOpts.ShouldDisplayDiagnosticName
+? " [" + PD->getCheckerName() + "]"
+: "")
+   .str();
 
   reportPiece(WarnID, PD->getLocation().asLocation(),
   (PD->getShortDescription() + WarningMsg).str(),
@@ -110,7 +108,7 @@
 Piece->getFixits());
   }
 
-  if (!IncludePath)
+  if (!ShouldDisplayPathNotes)
 continue;
 
   // Then, add the path notes if necessary.
@@ -125,7 +123,7 @@
   }
 }
 
-if (!ApplyFixIts || Repls.empty())
+if (Repls.empty())
   return;
 
 

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 277861.
NoQ added a comment.

Remove the `ShouldDisplayPathNotes` option as it never was an `AnalyzerOption` 
to begin with and it only makes things worse.

F12338321: 488ny6.jpg 


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

https://reviews.llvm.org/D67420

Files:
  clang/include/clang/Analysis/PathDiagnostic.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -150,7 +150,7 @@
   break;
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)\
   case PD_##NAME:  \
-CREATEFN(*Opts.get(), PathConsumers, OutDir, PP, CTU); \
+CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
 #include "clang/StaticAnalyzer/Core/Analyses.def"
 default:
Index: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
@@ -34,20 +34,17 @@
 /// type to the standard error, or to to compliment many others. Emits detailed
 /// diagnostics in textual format for the 'text' output type.
 class TextDiagnostics : public PathDiagnosticConsumer {
+  PathDiagnosticConsumerOptions DiagOpts;
   DiagnosticsEngine 
   const LangOptions 
-  const bool IncludePath = false;
-  const bool ShouldEmitAsError = false;
-  const bool ApplyFixIts = false;
-  const bool ShouldDisplayCheckerName = false;
+  bool ShouldDisplayPathNotes;
 
 public:
-  TextDiagnostics(DiagnosticsEngine , const LangOptions ,
-  bool ShouldIncludePath, const AnalyzerOptions )
-  : DiagEng(DiagEng), LO(LO), IncludePath(ShouldIncludePath),
-ShouldEmitAsError(AnOpts.AnalyzerWerror),
-ApplyFixIts(AnOpts.ShouldApplyFixIts),
-ShouldDisplayCheckerName(AnOpts.ShouldDisplayCheckerNameForText) {}
+  TextDiagnostics(PathDiagnosticConsumerOptions DiagOpts,
+  DiagnosticsEngine , const LangOptions ,
+  bool ShouldDisplayPathNotes)
+  : DiagOpts(std::move(DiagOpts)), DiagEng(DiagEng), LO(LO),
+ShouldDisplayPathNotes(ShouldDisplayPathNotes) {}
   ~TextDiagnostics() override {}
 
   StringRef getName() const override { return "TextDiagnostics"; }
@@ -56,13 +53,13 @@
   bool supportsCrossFileDiagnostics() const override { return true; }
 
   PathGenerationScheme getGenerationScheme() const override {
-return IncludePath ? Minimal : None;
+return ShouldDisplayPathNotes ? Minimal : None;
   }
 
   void FlushDiagnosticsImpl(std::vector ,
 FilesMade *filesMade) override {
 unsigned WarnID =
-ShouldEmitAsError
+DiagOpts.ShouldDisplayWarningsAsErrors
 ? DiagEng.getCustomDiagID(DiagnosticsEngine::Error, "%0")
 : DiagEng.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
 unsigned NoteID = DiagEng.getCustomDiagID(DiagnosticsEngine::Note, "%0");
@@ -72,7 +69,7 @@
 auto reportPiece = [&](unsigned ID, FullSourceLoc Loc, StringRef String,
ArrayRef Ranges,
ArrayRef Fixits) {
-  if (!ApplyFixIts) {
+  if (!DiagOpts.ShouldApplyFixIts) {
 DiagEng.Report(Loc, ID) << String << Ranges << Fixits;
 return;
   }
@@ -92,9 +89,10 @@
  E = Diags.end();
  I != E; ++I) {
   const PathDiagnostic *PD = *I;
-  std::string WarningMsg =
-  (ShouldDisplayCheckerName ? " [" + PD->getCheckerName() + "]" : "")
-  .str();
+  std::string WarningMsg = (DiagOpts.ShouldDisplayDiagnosticName
+? " [" + PD->getCheckerName() + "]"
+: "")
+   .str();
 
   reportPiece(WarnID, PD->getLocation().asLocation(),
   (PD->getShortDescription() + WarningMsg).str(),
@@ -110,7 +108,7 @@
 Piece->getFixits());
   }
 
-  if (!IncludePath)
+  if (!ShouldDisplayPathNotes)
 continue;
 
   // Then, add the path notes if necessary.
@@ -125,7 +123,7 @@
   }
 }
 
-if (!ApplyFixIts || Repls.empty())
+if (Repls.empty())
   return;
 
 Rewriter Rewrite(SM, LO);
@@ -139,18 

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

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

Szelethus accepted this revision.
Szelethus marked 2 inline comments as done.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM! Very well done!

In D67420#2149461 , @vsavchenko wrote:

> I strongly believe that decoupling (when done right) makes things easier and 
> this change seems like a good first step.


This patch I think categorizes, rather. We could do a much better job at that 
with these options, I agree, if we switched over to TableGen 
(D83475#inline-768468 ). The 
problem is, its hard to justify the engineering hours for an arguably 
negligible gain.




Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:47-48
+
+void printBugPath(llvm::raw_ostream , const FIDMap ,
+  const PathPieces );
+

Nice.



Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:39-42
-  const bool IncludePath = false;
-  const bool ShouldEmitAsError = false;
-  const bool ApplyFixIts = false;
-  const bool ShouldDisplayCheckerName = false;

Really nice!


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

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

Szelethus added a comment.

I've seen you resurrecting the thread, I'll just take a bit of time to remember 
what happened in the previous episodes!


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

I do like this change even apart from the `clang-tidy` integration epic.  I 
strongly believe that decoupling (when done right) makes things easier and this 
change seems like a good first step.


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

@vsavchenko and others just joining: The backstory and the overall plan are in 
this thread: http://lists.llvm.org/pipermail/cfe-dev/2019-August/063092.html. 
Continued at http://lists.llvm.org/pipermail/cfe-dev/2019-September/063229.html.


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a reviewer: vsavchenko.
NoQ added inline comments.



Comment at: clang/include/clang/Analysis/PathDiagnostic.h:81
+  /// because deterministic mode is always superior when done right, but
+  /// for some consumers is experimental and needs to be off by default.
+  bool ShouldWriteStableReportFilename;

Szelethus wrote:
> gribozavr wrote:
> > "but for some consumers is experimental" -- parse error. What is 
> > experimental?
> I suspect you copied these from the `.def` file, did you change the 
> descriptions there too?
Nope, these were hand-written. I tried to make sure they don't mention any 
static analyzer specific things.


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 277652.
NoQ marked 2 inline comments as done.
NoQ added a comment.
Herald added subscribers: ASDenysPetrov, martong.

Rebase!! This work was on hiatus but i plan to continue.

Address a review comment.

There are two new path diagnostic options since the last patch that correspond 
to the optional behaviors of the text consumer that were introduced since last 
revision:

- `ShouldApplyFixIts`;
- `ShouldDisplayDiagnosticName`.


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

https://reviews.llvm.org/D67420

Files:
  clang/include/clang/Analysis/PathDiagnostic.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -150,7 +150,7 @@
   break;
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)\
   case PD_##NAME:  \
-CREATEFN(*Opts.get(), PathConsumers, OutDir, PP, CTU); \
+CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
 #include "clang/StaticAnalyzer/Core/Analyses.def"
 default:
Index: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
@@ -34,20 +34,14 @@
 /// type to the standard error, or to to compliment many others. Emits detailed
 /// diagnostics in textual format for the 'text' output type.
 class TextDiagnostics : public PathDiagnosticConsumer {
+  PathDiagnosticConsumerOptions DiagOpts;
   DiagnosticsEngine 
   const LangOptions 
-  const bool IncludePath = false;
-  const bool ShouldEmitAsError = false;
-  const bool ApplyFixIts = false;
-  const bool ShouldDisplayCheckerName = false;
 
 public:
-  TextDiagnostics(DiagnosticsEngine , const LangOptions ,
-  bool ShouldIncludePath, const AnalyzerOptions )
-  : DiagEng(DiagEng), LO(LO), IncludePath(ShouldIncludePath),
-ShouldEmitAsError(AnOpts.AnalyzerWerror),
-ApplyFixIts(AnOpts.ShouldApplyFixIts),
-ShouldDisplayCheckerName(AnOpts.ShouldDisplayCheckerNameForText) {}
+  TextDiagnostics(PathDiagnosticConsumerOptions DiagOpts,
+  DiagnosticsEngine , const LangOptions )
+  : DiagOpts(std::move(DiagOpts)), DiagEng(DiagEng), LO(LO) {}
   ~TextDiagnostics() override {}
 
   StringRef getName() const override { return "TextDiagnostics"; }
@@ -56,13 +50,13 @@
   bool supportsCrossFileDiagnostics() const override { return true; }
 
   PathGenerationScheme getGenerationScheme() const override {
-return IncludePath ? Minimal : None;
+return DiagOpts.ShouldDisplayPathNotes ? Minimal : None;
   }
 
   void FlushDiagnosticsImpl(std::vector ,
 FilesMade *filesMade) override {
 unsigned WarnID =
-ShouldEmitAsError
+DiagOpts.ShouldDisplayWarningsAsErrors
 ? DiagEng.getCustomDiagID(DiagnosticsEngine::Error, "%0")
 : DiagEng.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
 unsigned NoteID = DiagEng.getCustomDiagID(DiagnosticsEngine::Note, "%0");
@@ -72,7 +66,7 @@
 auto reportPiece = [&](unsigned ID, FullSourceLoc Loc, StringRef String,
ArrayRef Ranges,
ArrayRef Fixits) {
-  if (!ApplyFixIts) {
+  if (!DiagOpts.ShouldApplyFixIts) {
 DiagEng.Report(Loc, ID) << String << Ranges << Fixits;
 return;
   }
@@ -92,9 +86,10 @@
  E = Diags.end();
  I != E; ++I) {
   const PathDiagnostic *PD = *I;
-  std::string WarningMsg =
-  (ShouldDisplayCheckerName ? " [" + PD->getCheckerName() + "]" : "")
-  .str();
+  std::string WarningMsg = (DiagOpts.ShouldDisplayDiagnosticName
+? " [" + PD->getCheckerName() + "]"
+: "")
+   .str();
 
   reportPiece(WarnID, PD->getLocation().asLocation(),
   (PD->getShortDescription() + WarningMsg).str(),
@@ -110,7 +105,7 @@
 Piece->getFixits());
   }
 
-  if (!IncludePath)
+  if (!DiagOpts.ShouldDisplayPathNotes)
 continue;
 
   // Then, add the path notes if necessary.
@@ -125,7 +120,7 @@
   }
 }
 
-if (!ApplyFixIts || 

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

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

Szelethus added inline comments.



Comment at: clang/include/clang/Analysis/PathDiagnostic.h:81
+  /// because deterministic mode is always superior when done right, but
+  /// for some consumers is experimental and needs to be off by default.
+  bool ShouldWriteStableReportFilename;

gribozavr wrote:
> "but for some consumers is experimental" -- parse error. What is experimental?
I suspect you copied these from the `.def` file, did you change the 
descriptions there too?


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

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

Szelethus added a comment.

Aha, so every user gets to create their own `PathDiagnosticConsumerOptions` 
object, makes sense! There is no interface misconception, because 
`-analyzer-config` will only configure what the analyzer would tinket with. I 
like this patch! If you dont mind, I'd prefer to have one last round with this, 
but otherwise LGTM.




Comment at: clang/include/clang/Analysis/PathDiagnostic.h:63
+/// Most of these options are currently supported by very few consumers.
+struct PathDiagnosticConsumerOptions {
+  /// Whether to include additional information about macro expansions

Lets delete the default constructor. It would be an option to generate this 
with the analyzer secific `.def` file, but since we have so few users, lets 
force them to create this object manually :)


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

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

Szelethus added a comment.

In D67420#1668474 , @NoQ wrote:

> It's not that `AnalyzerOptions` is //necessary// to construct this, it's more 
> like //sufficient//.


*sudden enlightenment*

Ah, okay, right, I'm a dummie, I think I got what's happening here! :) In that 
case, the idea sounds great. I'll take a second look on the code later (I have 
a feeling that we should delete the default ctor of 
`PathDiagnosticConsumerOptions`, etc), but I'm just a tad too tired atm :)


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D67420#1668099 , @Szelethus wrote:

> ...and the second is similar in nature, but in the actual code -- it doesn't 
> doesn't feel natural to me that `AnalyzerOptions` is required to construct 
> this, while at the same time we're trying to make diagnostics construction 
> independent of the analyzer.


It's not that `AnalyzerOptions` is //necessary// to construct this, it's more 
like //sufficient//. `PathDiagnosticConsumerOptions` is a plain de-encapsulated 
aggregate and anybody can aggregate-initialize or mutate it. But in the realm 
of the Analyzer it has "accidentally" turned out that `AnalyzerOptions` has 
just the right amount of information to fill it in.


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

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

Szelethus added a comment.

In D67420#1666578 , @NoQ wrote:

> In D67420#1666141 , @Szelethus wrote:
>
> > I do!
>
>
> Hmm, it sounds like you want to force both Clang frontend and Clang-Tidy to 
> use the same set of flags to control these options (?) Much like 
> `-analyzer-config`, these flags will have different "style" compared to other 
> flags in the tool. Which is probably not too bad for hidden frontend flags 
> that control the Analyzer because they're anyway set by a separate GUI 
> checkbox, but the inconsistency with other flags would be super glaring in 
> case of Clang-Tidy CLI. Do we really want to go in that direction? I'll be 
> much more comfortable with letting each tool deal with its flags the way it 
> prefers - this doesn't look like a good place for code reuse to me.


There are two things I wanted to touch on, but kinda failed to emphasize it. 
First is the topic of whether `-analyzer-config` flags are fitting for a 
feature no longer specific to the analyzer, and the second is similar in 
nature, but in the actual code -- it doesn't doesn't feel natural to me that 
`AnalyzerOptions` is required to construct this, while at the same time we're 
trying to make diagnostics construction independent of the analyzer. But I 
really wouldn't like to overengineer this just for the sake of it :^)

On the first, I'm kinda meh, even if we went for it, it would be a separate 
revision I guess, but the second has me concerned, unless I'm not following 
something right.


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

`PathDiagnosticConsumerOptions` is pretty lightweight, and is not passed around 
in hot paths if I understand correctly. What do you think about passing it by 
value everywhere?


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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 219826.
NoQ added a comment.

Clean up a tiny bit of dead code.


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

https://reviews.llvm.org/D67420

Files:
  clang/include/clang/Analysis/PathDiagnostic.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -65,19 +65,14 @@
 //===--===//
 
 void ento::createPlistHTMLDiagnosticConsumer(
-AnalyzerOptions , PathDiagnosticConsumers ,
+PathDiagnosticConsumerOptions &, PathDiagnosticConsumers ,
 const std::string , const Preprocessor ,
 const cross_tu::CrossTranslationUnitContext ) {
-  createHTMLDiagnosticConsumer(AnalyzerOpts, C,
+  // Duplicate the DiagOpts object into both consumers.
+  createHTMLDiagnosticConsumer(PathDiagnosticConsumerOptions(DiagOpts), C,
llvm::sys::path::parent_path(prefix), PP, CTU);
-  createPlistMultiFileDiagnosticConsumer(AnalyzerOpts, C, prefix, PP, CTU);
-}
-
-void ento::createTextPathDiagnosticConsumer(
-AnalyzerOptions , PathDiagnosticConsumers ,
-const std::string , const clang::Preprocessor ,
-const cross_tu::CrossTranslationUnitContext ) {
-  llvm_unreachable("'text' consumer should be enabled on ClangDiags");
+  createPlistMultiFileDiagnosticConsumer(
+  PathDiagnosticConsumerOptions(DiagOpts), C, prefix, PP, CTU);
 }
 
 namespace {
@@ -86,8 +81,11 @@
   bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false;
 
 public:
-  ClangDiagPathDiagConsumer(DiagnosticsEngine )
-  : Diag(Diag) {}
+  ClangDiagPathDiagConsumer(PathDiagnosticConsumerOptions &,
+DiagnosticsEngine )
+  : Diag(Diag), IncludePath(DiagOpts.ShouldDisplayPathNotes),
+ShouldEmitAsError(DiagOpts.ShouldDisplayWarningsAsErrors),
+FixitsAsRemarks(DiagOpts.ShouldEmitFixItHintsAsRemarks) {}
   ~ClangDiagPathDiagConsumer() override {}
   StringRef getName() const override { return "ClangDiags"; }
 
@@ -98,10 +96,6 @@
 return IncludePath ? Minimal : None;
   }
 
-  void enablePaths() { IncludePath = true; }
-  void enableWerror() { ShouldEmitAsError = true; }
-  void enableFixitsAsRemarks() { FixitsAsRemarks = true; }
-
   void FlushDiagnosticsImpl(std::vector ,
 FilesMade *filesMade) override {
 unsigned WarnID =
@@ -168,6 +162,14 @@
 };
 } // end anonymous namespace
 
+void ento::createTextPathDiagnosticConsumer(
+PathDiagnosticConsumerOptions &, PathDiagnosticConsumers ,
+const std::string , const clang::Preprocessor ,
+const cross_tu::CrossTranslationUnitContext ) {
+  C.push_back(
+  new ClangDiagPathDiagConsumer(std::move(DiagOpts), PP.getDiagnostics()));
+}
+
 //===--===//
 // AnalysisConsumer declaration.
 //===--===//
@@ -254,26 +256,21 @@
 
   void DigestAnalyzerOptions() {
 if (Opts->AnalysisDiagOpt != PD_NONE) {
-  // Create the PathDiagnosticConsumer.
-  ClangDiagPathDiagConsumer *clangDiags =
-  new ClangDiagPathDiagConsumer(PP.getDiagnostics());
-  PathConsumers.push_back(clangDiags);
-
-  if (Opts->AnalyzerWerror)
-clangDiags->enableWerror();
-
-  if (Opts->ShouldEmitFixItHintsAsRemarks)
-clangDiags->enableFixitsAsRemarks();
-
-  if (Opts->AnalysisDiagOpt == PD_TEXT) {
-clangDiags->enablePaths();
-
-  } else if (!OutDir.empty()) {
+  // Create the text consumer unconditionally. It will display warnings
+  // on the console even if other consumers display the warning to the user
+  // in a more sophisticated format.
+  PathDiagnosticConsumerOptions TextDiagOpts = Opts->getDiagOpts();
+  TextDiagOpts.ShouldDisplayPathNotes = (Opts->AnalysisDiagOpt == PD_TEXT);
+  createTextPathDiagnosticConsumer(std::move(TextDiagOpts), PathConsumers,
+   OutDir, PP, CTU);
+
+  // Create other consumers if requested.
+  if (Opts->AnalysisDiagOpt != PD_TEXT  && !OutDir.empty()) {
 switch (Opts->AnalysisDiagOpt) {
 default:
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)\
   case PD_##NAME:  \
-CREATEFN(*Opts.get(), PathConsumers, OutDir, PP, CTU); 

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 219821.
NoQ added a comment.

Unforget to do the same for the text consumer. As a side effect, the factory 
function for the text consumer is no longer special, which will be less 
confusing when put in libAnalysis.

Address minor comments, don't address major comments.


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

https://reviews.llvm.org/D67420

Files:
  clang/include/clang/Analysis/PathDiagnostic.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -65,19 +65,14 @@
 //===--===//
 
 void ento::createPlistHTMLDiagnosticConsumer(
-AnalyzerOptions , PathDiagnosticConsumers ,
+PathDiagnosticConsumerOptions &, PathDiagnosticConsumers ,
 const std::string , const Preprocessor ,
 const cross_tu::CrossTranslationUnitContext ) {
-  createHTMLDiagnosticConsumer(AnalyzerOpts, C,
+  // Duplicate the DiagOpts object into both consumers.
+  createHTMLDiagnosticConsumer(PathDiagnosticConsumerOptions(DiagOpts), C,
llvm::sys::path::parent_path(prefix), PP, CTU);
-  createPlistMultiFileDiagnosticConsumer(AnalyzerOpts, C, prefix, PP, CTU);
-}
-
-void ento::createTextPathDiagnosticConsumer(
-AnalyzerOptions , PathDiagnosticConsumers ,
-const std::string , const clang::Preprocessor ,
-const cross_tu::CrossTranslationUnitContext ) {
-  llvm_unreachable("'text' consumer should be enabled on ClangDiags");
+  createPlistMultiFileDiagnosticConsumer(
+  PathDiagnosticConsumerOptions(DiagOpts), C, prefix, PP, CTU);
 }
 
 namespace {
@@ -86,8 +81,11 @@
   bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false;
 
 public:
-  ClangDiagPathDiagConsumer(DiagnosticsEngine )
-  : Diag(Diag) {}
+  ClangDiagPathDiagConsumer(PathDiagnosticConsumerOptions &,
+DiagnosticsEngine )
+  : Diag(Diag), IncludePath(DiagOpts.ShouldDisplayPathNotes),
+ShouldEmitAsError(DiagOpts.ShouldDisplayWarningsAsErrors),
+FixitsAsRemarks(DiagOpts.ShouldEmitFixItHintsAsRemarks) {}
   ~ClangDiagPathDiagConsumer() override {}
   StringRef getName() const override { return "ClangDiags"; }
 
@@ -168,6 +166,14 @@
 };
 } // end anonymous namespace
 
+void ento::createTextPathDiagnosticConsumer(
+PathDiagnosticConsumerOptions &, PathDiagnosticConsumers ,
+const std::string , const clang::Preprocessor ,
+const cross_tu::CrossTranslationUnitContext ) {
+  C.push_back(
+  new ClangDiagPathDiagConsumer(std::move(DiagOpts), PP.getDiagnostics()));
+}
+
 //===--===//
 // AnalysisConsumer declaration.
 //===--===//
@@ -254,26 +260,21 @@
 
   void DigestAnalyzerOptions() {
 if (Opts->AnalysisDiagOpt != PD_NONE) {
-  // Create the PathDiagnosticConsumer.
-  ClangDiagPathDiagConsumer *clangDiags =
-  new ClangDiagPathDiagConsumer(PP.getDiagnostics());
-  PathConsumers.push_back(clangDiags);
-
-  if (Opts->AnalyzerWerror)
-clangDiags->enableWerror();
-
-  if (Opts->ShouldEmitFixItHintsAsRemarks)
-clangDiags->enableFixitsAsRemarks();
-
-  if (Opts->AnalysisDiagOpt == PD_TEXT) {
-clangDiags->enablePaths();
-
-  } else if (!OutDir.empty()) {
+  // Create the text consumer unconditionally. It will display warnings
+  // on the console even if other consumers display the warning to the user
+  // in a more sophisticated format.
+  PathDiagnosticConsumerOptions TextDiagOpts = Opts->getDiagOpts();
+  TextDiagOpts.ShouldDisplayPathNotes = (Opts->AnalysisDiagOpt == PD_TEXT);
+  createTextPathDiagnosticConsumer(std::move(TextDiagOpts), PathConsumers,
+   OutDir, PP, CTU);
+
+  // Create other consumers if requested.
+  if (Opts->AnalysisDiagOpt != PD_TEXT  && !OutDir.empty()) {
 switch (Opts->AnalysisDiagOpt) {
 default:
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)\
   case PD_##NAME:  \
-CREATEFN(*Opts.get(), PathConsumers, OutDir, PP, CTU); \
+CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
 #include "clang/StaticAnalyzer/Core/Analyses.def"

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D67420#1666050 , @gribozavr wrote:

> Are you planning to move users of these options -- like `PlistDiagnostics` -- 
> to libAnalysis?


Yup, i was supposed to do that in D67422  :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D67420#1666141 , @Szelethus wrote:

> - For these select 4 options that suffer compatibility issues, manually check 
> `AnalyzerOptions::ConfigTable`.


*3 options. `ToolInvocation` is not really an option that you're ever supposed 
to adjust manually. Which is one more reason for me to think of this structure 
as of an implementation detail - an interface through which diagnostic 
consumers' behavior can be tweaked, regardless of why the tool wants it to be 
tweaked (because the human told it to or because the tool itself knows better).


Repository:
  rC Clang

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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D67420#1666141 , @Szelethus wrote:

> I do!


Hmm, it sounds like you want to force both Clang frontend and Clang-Tidy to use 
the same set of flags to control these options (?) Much like 
`-analyzer-config`, these flags will have different "style" compared to other 
flags in the tool. Which is probably not too bad for hidden frontend flags that 
control the Analyzer because they're anyway set by a separate GUI checkbox, but 
the inconsistency with other flags would be super glaring in case of Clang-Tidy 
CLI. Do we really want to go in that direction? I'll be much more comfortable 
with letting each tool deal with its flags the way it prefers - this doesn't 
look like a good place for code reuse to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

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

Szelethus added a comment.

> @Szelethus: I could have stored `PathDiagnosticConsumerOptions` in 
> `AnalyzerOptions` by value and pass a const reference around, but it wasn't 
> pleasant to integrate with `AnalyzerOptions.def`. I.e., i'd have to implement 
> a new kind of option that doesn't allocate its own field but instead re-uses 
> a field within a sub-object. Do you want me to go for it or is this 
> implementation good enough?

I don't really like this direction :^) Imagine how tedious and non-obvious 
it'll be to add a new pathdiagnostic option. I don't like we're copying this 
around. There is also a discussion to be had about whether having to specify 
`-analyzer-config` to a no longer analyzer specific feature is any good.

> or do you have other approaches in mind?

I do! The level idea is to completely separate pathdiagnostic configs from the 
`AnalyzerOptions`, I believe this is more in line with your goal.

- Let's create the equivalent of `-analyzer-config`, `-pathdiagnostic-config`, 
and things like `-pathdiagnostic-config-help`.
- Avoid getting a parsing error for specifying `-analyzer-config 
macro-expansion` instead of  `-pathdiagnostic-config macro-expansion`.
  - Create a field similar to `AnalyzerOptions::AnalyzerConfigCmdFlags` called 
`AnalyzerOptions::AnalyzerConfigAliases` with the 4 configs that we're moving 
over. In compatibility mode, `AnalyzerOptions::isUnknownAnalyzerConfig()` shall 
search in this field as well. In non-compatibility mode, this will result in an 
error.
- Make `PathDiagnosticConsumerOptions` field of `CompilerInvocation`, similar 
to `AnalyzerOptions`.
- Your idea of implementing a `DIAGNOSTIC_OPTION` macro sounds nice.
  - Delete these entries from `AnalyzerOptions.def`.
  - Create `clang/include/Analysis/PathDiagnostics.def`, list them there.
  - Generate fields to `PathDiagnosticConsumerOptions` similar to how 
`AnalyzerOptions` does it.
- Parse the options `CompilerInvocation.cpp`
  - Most of the analyzer config parsing functions can be reused!
  - For these select 4 options that suffer compatibility issues, manually check 
`AnalyzerOptions::ConfigTable`.

WDYT? I am happy to help out, I realize this is plenty of code to write, if we 
go for it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Are you planning to move users of these options -- like `PlistDiagnostics` -- 
to libAnalysis?




Comment at: clang/include/clang/Analysis/PathDiagnostic.h:81
+  /// because deterministic mode is always superior when done right, but
+  /// for some consumers is experimental and needs to be off by default.
+  bool ShouldWriteStableReportFilename;

"but for some consumers is experimental" -- parse error. What is experimental?



Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:534
+  C.push_back(new PlistDiagnostics(std::move(DiagOpts), s, PP, CTU,
+   /*supportsMultipleFiles*/ false));
+}

I think we need an equal sign for ClangTidy to pick it up: `/*foo=*/`


Repository:
  rC Clang

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

https://reviews.llvm.org/D67420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, 
baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, 
a.sidorin, szepet.
Herald added a project: clang.
NoQ added reviewers: alexfh, gribozavr.
NoQ added a parent revision: D67419: [analyzer] NFC: Move PathDiagnostic to 
libAnalysis..

The `AnalyzerOptions` object contains too much information that's completely 
specific to the Analyzer. It is also being referenced by path diagnostic 
consumers to tweak their behavior. If we want path diagnostic consumers to 
function separately from the Analyzer, we'll have to make a smaller options 
object that only contains relevant options.

@Szelethus: I could have stored `PathDiagnosticConsumerOptions` in 
`AnalyzerOptions` by value and pass a const reference around, but it wasn't 
pleasant to integrate with `AnalyzerOptions.def`. I.e., i'd have to implement a 
new kind of option that doesn't allocate its own field but instead re-uses a 
field within a sub-object. Do you want me to go for it or is this 
implementation good enough or do you have other approaches in mind?


Repository:
  rC Clang

https://reviews.llvm.org/D67420

Files:
  clang/include/clang/Analysis/PathDiagnostic.h
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -65,16 +65,18 @@
 //===--===//
 
 void ento::createPlistHTMLDiagnosticConsumer(
-AnalyzerOptions , PathDiagnosticConsumers ,
+PathDiagnosticConsumerOptions &, PathDiagnosticConsumers ,
 const std::string , const Preprocessor ,
 const cross_tu::CrossTranslationUnitContext ) {
-  createHTMLDiagnosticConsumer(AnalyzerOpts, C,
+  // Duplicate the DiagOpts object into both consumers.
+  createHTMLDiagnosticConsumer(PathDiagnosticConsumerOptions(DiagOpts), C,
llvm::sys::path::parent_path(prefix), PP, CTU);
-  createPlistMultiFileDiagnosticConsumer(AnalyzerOpts, C, prefix, PP, CTU);
+  createPlistMultiFileDiagnosticConsumer(
+  PathDiagnosticConsumerOptions(DiagOpts), C, prefix, PP, CTU);
 }
 
 void ento::createTextPathDiagnosticConsumer(
-AnalyzerOptions , PathDiagnosticConsumers ,
+PathDiagnosticConsumerOptions &, PathDiagnosticConsumers ,
 const std::string , const clang::Preprocessor ,
 const cross_tu::CrossTranslationUnitContext ) {
   llvm_unreachable("'text' consumer should be enabled on ClangDiags");
@@ -273,7 +275,7 @@
 default:
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)\
   case PD_##NAME:  \
-CREATEFN(*Opts.get(), PathConsumers, OutDir, PP, CTU); \
+CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
 #include "clang/StaticAnalyzer/Core/Analyses.def"
 }
Index: clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
@@ -13,7 +13,6 @@
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Basic/Version.h"
 #include "clang/Lex/Preprocessor.h"
-#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
@@ -29,7 +28,7 @@
   std::string OutputFile;
 
 public:
-  SarifDiagnostics(AnalyzerOptions &, const std::string )
+  SarifDiagnostics(const std::string )
   : OutputFile(Output) {}
   ~SarifDiagnostics() override = default;
 
@@ -44,10 +43,10 @@
 } // end anonymous namespace
 
 void ento::createSarifDiagnosticConsumer(
-AnalyzerOptions , PathDiagnosticConsumers ,
+PathDiagnosticConsumerOptions &, PathDiagnosticConsumers ,
 const std::string , const Preprocessor &,
 const cross_tu::CrossTranslationUnitContext &) {
-  C.push_back(new SarifDiagnostics(AnalyzerOpts, Output));
+  C.push_back(new SarifDiagnostics(Output));
 }
 
 static StringRef getFileName(const FileEntry ) {
Index: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -20,7 +20,6 @@