[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ abandoned this revision.
NoQ added a comment.

Abandon in favor of splitting up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ reopened this revision.
NoQ added a comment.
Herald added a subscriber: manas.

This patch hasn't landed yet (reverted every time due to circular 
dependencies(?)). I plan to split it up into smaller patches as I rebase 
because rebase is already very painful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp:307
   .Case(FULLNAME, HELPTEXT)
 #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
 #undef CHECKER

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > This thing still worries me but this definitely isn't a link-time 
> > > > dependency.
> > > D53277#1285757, rGb8cfcc71469d40a98f4cc79fcdc46cd67bea45f7
> > Ok, what's the proper solution here? This is clearly a layering violation 
> > now; this generic diagnostic consumer shouldn't know anything about the 
> > Static Analyzer specifically. I guess we could separate it into an 
> > independent polymorphic object ("DescriptionGetter" or something like that) 
> > that the Static Analyzer would instantiate manually. Or ideally we could 
> > ship this information with the bug report itself.
> > 
> > I'll add a FIXME and try to reproduce potential modules problems locally.
> I am puzzled myself :/ Maybe we could ask @aaron.ballman, since he landed 
> most of these changes back in the day?
The original code was added because it's needed for the required SARIF output. 
At the time, it didn't seem like it was a layering violation because the SARIF 
output was limited to just the clang static analyzer diagnostics -- however, I 
agree that work needs to be done here now. For instance, one of the issues with 
the SARIF output is that it only captures output from the static analyzer and 
doesn't report diagnostics from Clang itself, IIRC. We'll need to solve this 
for clang-tidy diagnostics as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp:307
   .Case(FULLNAME, HELPTEXT)
 #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
 #undef CHECKER

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > This thing still worries me but this definitely isn't a link-time 
> > > > dependency.
> > > D53277#1285757, rGb8cfcc71469d40a98f4cc79fcdc46cd67bea45f7
> > Ok, what's the proper solution here? This is clearly a layering violation 
> > now; this generic diagnostic consumer shouldn't know anything about the 
> > Static Analyzer specifically. I guess we could separate it into an 
> > independent polymorphic object ("DescriptionGetter" or something like that) 
> > that the Static Analyzer would instantiate manually. Or ideally we could 
> > ship this information with the bug report itself.
> > 
> > I'll add a FIXME and try to reproduce potential modules problems locally.
> I am puzzled myself :/ Maybe we could ask @aaron.ballman, since he landed 
> most of these changes back in the day?
I fixed a couple other bugs that caused modules build to fail before pushing 
but this one wasn't a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG44b7cf2983b6: [analyzer] NFC: Move path diagnostic consumer 
implementations to libAnalysis. (authored by dergachev.a).

Changed prior to commit:
  https://reviews.llvm.org/D67422?vs=285417&id=297910#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67422

Files:
  clang/include/clang/Analysis/PathDiagnosticConsumers.def
  clang/include/clang/Analysis/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/Analyses.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
  clang/include/clang/module.modulemap
  clang/lib/Analysis/CMakeLists.txt
  clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistPathDiagnosticConsumer.cpp
  clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
  clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  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
@@ -21,6 +21,7 @@
 #include "clang/Analysis/CallGraph.h"
 #include "clang/Analysis/CodeInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -30,7 +31,6 @@
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/PostOrderIterator.h"
@@ -152,7 +152,7 @@
   case PD_##NAME:  \
 CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
 default:
   llvm_unreachable("Unknown analyzer output type!");
 }
Index: clang/lib/StaticAnalyzer/Core/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Core/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -30,16 +30,13 @@
   ExprEngineCallAndReturn.cpp
   ExprEngineObjC.cpp
   FunctionSummary.cpp
-  HTMLDiagnostics.cpp
   LoopUnrolling.cpp
   LoopWidening.cpp
   MemRegion.cpp
-  PlistDiagnostics.cpp
   ProgramState.cpp
   RangeConstraintManager.cpp
   RangedConstraintManager.cpp
   RegionStore.cpp
-  SarifDiagnostics.cpp
   SimpleConstraintManager.cpp
   SimpleSValBuilder.cpp
   SMTConstraintManager.cpp
@@ -47,7 +44,6 @@
   SValBuilder.cpp
   SVals.cpp
   SymbolManager.cpp
-  TextDiagnostics.cpp
   WorkList.cpp
 
   LINK_LIBS
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -301,7 +301,7 @@
 AnalysisDiagClients Value = llvm::StringSwitch(Name)
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATFN) \
   .Case(CMDFLAG, PD_##NAME)
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
   .Default(NUM_ANALYSIS_DIAG_CLIENTS);
 if (Value == NUM_ANALYSIS_DIAG_CLIENTS) {
   Diags.Report(diag::err_drv_invalid_value)
Index: clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
===
--- clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
+++ clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
@@ -1,4 +1,4 @@
-//===--- TextDiagnostics.cpp - Text Diagnostics for Paths ---*- C++ -*-===//
+//===--- TextPathDiagnosticConsumer.cpp - Text Diagnostics --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,19 +6,18 @@
 //
 //===-

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

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

Szelethus added a subscriber: aaron.ballman.
Szelethus added inline comments.



Comment at: clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp:307
   .Case(FULLNAME, HELPTEXT)
 #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
 #undef CHECKER

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > This thing still worries me but this definitely isn't a link-time 
> > > dependency.
> > D53277#1285757, rGb8cfcc71469d40a98f4cc79fcdc46cd67bea45f7
> Ok, what's the proper solution here? This is clearly a layering violation 
> now; this generic diagnostic consumer shouldn't know anything about the 
> Static Analyzer specifically. I guess we could separate it into an 
> independent polymorphic object ("DescriptionGetter" or something like that) 
> that the Static Analyzer would instantiate manually. Or ideally we could ship 
> this information with the bug report itself.
> 
> I'll add a FIXME and try to reproduce potential modules problems locally.
I am puzzled myself :/ Maybe we could ask @aaron.ballman, since he landed most 
of these changes back in the day?


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp:307
   .Case(FULLNAME, HELPTEXT)
 #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
 #undef CHECKER

Szelethus wrote:
> NoQ wrote:
> > This thing still worries me but this definitely isn't a link-time 
> > dependency.
> D53277#1285757, rGb8cfcc71469d40a98f4cc79fcdc46cd67bea45f7
Ok, what's the proper solution here? This is clearly a layering violation now; 
this generic diagnostic consumer shouldn't know anything about the Static 
Analyzer specifically. I guess we could separate it into an independent 
polymorphic object ("DescriptionGetter" or something like that) that the Static 
Analyzer would instantiate manually. Or ideally we could ship this information 
with the bug report itself.

I'll add a FIXME and try to reproduce potential modules problems locally.


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

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

Szelethus added a comment.

In D67422#2216137 , @NoQ wrote:

> I agree that there's something here and also that it's not that 
> urgent/critical to rename things at this point. It's not that people suffer 
> horribly from having to link to only some of these things.

In the long run, if we end up piling more of the common stuff in `libAnalysis`, 
it might be worth to create a new library.


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

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

Szelethus accepted this revision.
Szelethus added a comment.

Unless anyone else objects, I'm happy to see this landed! Nice work! ^-^


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

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

Szelethus added inline comments.



Comment at: clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp:307
   .Case(FULLNAME, HELPTEXT)
 #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
 #undef CHECKER

NoQ wrote:
> This thing still worries me but this definitely isn't a link-time dependency.
D53277#1285757, rGb8cfcc71469d40a98f4cc79fcdc46cd67bea45f7


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp:307
   .Case(FULLNAME, HELPTEXT)
 #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
 #undef CHECKER

This thing still worries me but this definitely isn't a link-time dependency.


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

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

In D67422#2215371 , @Szelethus wrote:

> The patch looks great. I guess the only remaining discussion remains as to 
> whether this should be in `libAnalysis`, or somewhere else. Here is my take: 
> Since clang-tidy, CSA, and some other parts of the compiler (like 
> uninitialized variable warnings) are static analyzers within the same 
> infrastructure, it makes sense for them to have a common library. I see that 
> diagnostics aren't really analyses. `libFrontend` or `libDriver`, which, as I 
> understand it contains most the diagnostics machinery for clang aren't viable 
> alternatives because of the library dependencies. So, I think if 
> `libAnalysis` was called `libStaticAnalysisCommon`, we would be golden, but 
> that would screw over downstream developers for negligible gain. While I'm 
> not terribly experiences in library layout design, for the time being, the 
> move to `libAnalysis` seems appropriate.

I agree that there's something here and also that it's not that urgent/critical 
to rename things at this point. It's not that people suffer horribly from 
having to link to only some of these things.

> I see that we're still heavily tied to StaticAnalyzer headers. Doesn't that 
> violate the module system, as `libAnalysis` is a dependency of 
> `libStaticAnalyzerCore`? `AnalyzerOptions` and `AnalysisConsumer` (that last 
> one in particular) seem deeply integrated into these consumers.

Mmm, these are dead includes now. Reliance on `AnalyzerOptions` was supposed to 
be eliminated since D67420  and these are in 
fact rebase conflicts. Fxd.


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

https://reviews.llvm.org/D67422

Files:
  clang/include/clang/Analysis/PathDiagnosticConsumers.def
  clang/include/clang/Analysis/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/Analyses.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
  clang/lib/Analysis/CMakeLists.txt
  clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistPathDiagnosticConsumer.cpp
  clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
  clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  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
@@ -21,6 +21,7 @@
 #include "clang/Analysis/CallGraph.h"
 #include "clang/Analysis/CodeInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -30,7 +31,6 @@
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/PostOrderIterator.h"
@@ -152,7 +152,7 @@
   case PD_##NAME:  \
 CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
 default:
   llvm_unreachable("Unknown analyzer output type!");
 }
Index: clang/lib/StaticAnalyzer/Core/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Core/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -30,16 +30,13 @@
   ExprEngineCallAndReturn.cpp
   ExprEngineObjC.cpp
   FunctionSummary.cpp
-  HTMLDiagnostics.cpp
   LoopUnrolling.cpp
   LoopWidening.cpp
   MemRegion.cpp
-  PlistDiagnostics.cpp
   ProgramState.cpp
   RangeConstraintManager.cpp
   RangedConstraintManager.cpp
   RegionStore.cpp
-  SarifDiagnostics.cpp
   SimpleConstraintManager.cpp
   SimpleSValBuilder.cpp
   SMTConstraintManager.cpp
@@ -47,7 +44,6 @@
   SValBuilder.cpp
   SVals.cpp
   SymbolManager.cpp
-  TextDiagnostics.cpp
   WorkList.cpp
 
   LINK_L

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko accepted this revision.
vsavchenko added a comment.

Awesome, looks great!


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

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

Szelethus added a comment.

The patch looks great. I guess the only remaining discussion remains as to 
whether this should be in `libAnalysis`, or somewhere else. Here is my take: 
Since clang-tidy, CSA, and some other parts of the compiler (like uninitialized 
variable warnings) are static analyzers within the same infrastructure, it 
makes sense for them to have a common library. I see that diagnostics aren't 
really analyses. `libFrontend` or `libDriver`, which, as I understand it 
contains most the diagnostics machinery for clang aren't viable alternatives 
because of the library dependencies. So, I think if `libAnalysis` was called 
`libStaticAnalysisCommon`, we would be golden, but that would screw over 
downstream developers for negligible gain. While I'm not terribly experiences 
in library layout design, for the time being, the move to `libAnalysis` seems 
appropriate.

I see that we're still heavily tied to StaticAnalyzer headers. Doesn't that 
violate the module system, as `libAnalysis` is a dependency of 
`libStaticAnalyzerCore`? `AnalyzerOptions` and `AnalysisConsumer` (that last 
one in particular) seem deeply integrated into these consumers.




Comment at: clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp:27-31
+  createHTMLDiagnosticConsumer(
+  DiagOpts, C, std::string(llvm::sys::path::parent_path(Prefix)), PP, CTU);
+  createPlistMultiFileDiagnosticConsumer(DiagOpts, C, Prefix, PP, CTU);
+  createTextMinimalPathDiagnosticConsumer(std::move(DiagOpts), C, Prefix, PP,
+  CTU);

NoQ wrote:
> I just realized that this code creates 5 consumers: 1 html consumer, 1 plist 
> consumer, 3 text consumers. This isn't a regression due to this patch... but 
> damn!
Oh wow. That is definitely my mistake. The issue is that detailed diagnostics 
(`-analyzer-output=text`) and the minimal one (`analyzer-output=text-minimal`) 
should be mutually exclusive. I'll try to think about a solution. Maybe some 
`Profile` thing might work.


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp:27-31
+  createHTMLDiagnosticConsumer(
+  DiagOpts, C, std::string(llvm::sys::path::parent_path(Prefix)), PP, CTU);
+  createPlistMultiFileDiagnosticConsumer(DiagOpts, C, Prefix, PP, CTU);
+  createTextMinimalPathDiagnosticConsumer(std::move(DiagOpts), C, Prefix, PP,
+  CTU);

I just realized that this code creates 5 consumers: 1 html consumer, 1 plist 
consumer, 3 text consumers. This isn't a regression due to this patch... but 
damn!


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 283993.
NoQ marked 2 inline comments as done.
NoQ added a comment.

Whoops fxd!


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

https://reviews.llvm.org/D67422

Files:
  clang/include/clang/Analysis/PathDiagnosticConsumers.def
  clang/include/clang/Analysis/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/Analyses.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
  clang/lib/Analysis/CMakeLists.txt
  clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistPathDiagnosticConsumer.cpp
  clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
  clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  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
@@ -21,6 +21,7 @@
 #include "clang/Analysis/CallGraph.h"
 #include "clang/Analysis/CodeInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -30,7 +31,6 @@
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/PostOrderIterator.h"
@@ -152,7 +152,7 @@
   case PD_##NAME:  \
 CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
 default:
   llvm_unreachable("Unknown analyzer output type!");
 }
Index: clang/lib/StaticAnalyzer/Core/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Core/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -30,16 +30,13 @@
   ExprEngineCallAndReturn.cpp
   ExprEngineObjC.cpp
   FunctionSummary.cpp
-  HTMLDiagnostics.cpp
   LoopUnrolling.cpp
   LoopWidening.cpp
   MemRegion.cpp
-  PlistDiagnostics.cpp
   ProgramState.cpp
   RangeConstraintManager.cpp
   RangedConstraintManager.cpp
   RegionStore.cpp
-  SarifDiagnostics.cpp
   SimpleConstraintManager.cpp
   SimpleSValBuilder.cpp
   SMTConstraintManager.cpp
@@ -47,7 +44,6 @@
   SValBuilder.cpp
   SVals.cpp
   SymbolManager.cpp
-  TextDiagnostics.cpp
   WorkList.cpp
 
   LINK_LIBS
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -301,7 +301,7 @@
 AnalysisDiagClients Value = llvm::StringSwitch(Name)
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATFN) \
   .Case(CMDFLAG, PD_##NAME)
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
   .Default(NUM_ANALYSIS_DIAG_CLIENTS);
 if (Value == NUM_ANALYSIS_DIAG_CLIENTS) {
   Diags.Report(diag::err_drv_invalid_value)
Index: clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
===
--- clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
+++ clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
@@ -1,4 +1,4 @@
-//===--- TextDiagnostics.cpp - Text Diagnostics for Paths ---*- C++ -*-===//
+//===--- TextPathDiagnosticConsumer.cpp - Text Diagnostics --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,11 +6,12 @@
 //
 //===--===//
 //
-//  This file defines the TextDiagnostics object.
+//  This file defines the TextPathDiagnosticConsumer object.
 //
 //===--===//
 
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/Analysis/TextPathDiagnosticConsumer.cpp:1
 //===--- TextDiagnostics.cpp - Text Diagnostics for Paths ---*- C++ 
-*-===//
 //

TextPathDiagnosticConsumer.cpp?



Comment at: clang/lib/Analysis/TextPathDiagnosticConsumer.cpp:36
 /// diagnostics in textual format for the 'text' output type.
 class TextDiagnostics : public PathDiagnosticConsumer {
   PathDiagnosticConsumerOptions DiagOpts;

`TextPathDiagnosticConsumer`?


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 283461.
NoQ marked 2 inline comments as done.
NoQ added a comment.

Fxd.


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

https://reviews.llvm.org/D67422

Files:
  clang/include/clang/Analysis/PathDiagnosticConsumers.def
  clang/include/clang/Analysis/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/Analyses.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
  clang/lib/Analysis/CMakeLists.txt
  clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistPathDiagnosticConsumer.cpp
  clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
  clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  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
@@ -21,6 +21,7 @@
 #include "clang/Analysis/CallGraph.h"
 #include "clang/Analysis/CodeInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -30,7 +31,6 @@
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/PostOrderIterator.h"
@@ -152,7 +152,7 @@
   case PD_##NAME:  \
 CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
 default:
   llvm_unreachable("Unknown analyzer output type!");
 }
Index: clang/lib/StaticAnalyzer/Core/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Core/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -30,16 +30,13 @@
   ExprEngineCallAndReturn.cpp
   ExprEngineObjC.cpp
   FunctionSummary.cpp
-  HTMLDiagnostics.cpp
   LoopUnrolling.cpp
   LoopWidening.cpp
   MemRegion.cpp
-  PlistDiagnostics.cpp
   ProgramState.cpp
   RangeConstraintManager.cpp
   RangedConstraintManager.cpp
   RegionStore.cpp
-  SarifDiagnostics.cpp
   SimpleConstraintManager.cpp
   SimpleSValBuilder.cpp
   SMTConstraintManager.cpp
@@ -47,7 +44,6 @@
   SValBuilder.cpp
   SVals.cpp
   SymbolManager.cpp
-  TextDiagnostics.cpp
   WorkList.cpp
 
   LINK_LIBS
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -301,7 +301,7 @@
 AnalysisDiagClients Value = llvm::StringSwitch(Name)
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATFN) \
   .Case(CMDFLAG, PD_##NAME)
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
   .Default(NUM_ANALYSIS_DIAG_CLIENTS);
 if (Value == NUM_ANALYSIS_DIAG_CLIENTS) {
   Diags.Report(diag::err_drv_invalid_value)
Index: clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
===
--- clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
+++ clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
@@ -18,7 +19,6 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/SmallPtrSet.h"
Index: clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
==

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp:25
+PathDiagnosticConsumerOptions DiagOpts, PathDiagnosticConsumers &C,
+const std::string &prefix, const Preprocessor &PP,
+const cross_tu::CrossTranslationUnitContext &CTU) {

vsavchenko wrote:
> As long as it is a new function, I guess we should definitely keep names 
> according to the **Coding Standards**
It's not new but sure!


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Other than parameter names, it looks totally reasonable to me.




Comment at: clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp:70
+ const std::string &OutputDir,
+ const Preprocessor &pp, bool 
supportsMultipleFiles)
   : DiagOpts(std::move(DiagOpts)), Directory(OutputDir), PP(pp),

Maybe while we are here, we can change the names to match the code style (aka 
capitalize)?
It seems like all other functions have similar problems.  It's no biggie, but 
still...

NOTE: I really wanted to try this new feature of Phabricator 😊 



Comment at: clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp:25
+PathDiagnosticConsumerOptions DiagOpts, PathDiagnosticConsumers &C,
+const std::string &prefix, const Preprocessor &PP,
+const cross_tu::CrossTranslationUnitContext &CTU) {

As long as it is a new function, I guess we should definitely keep names 
according to the **Coding Standards**


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

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

Szelethus added a comment.

In D67422#2192407 , @NoQ wrote:

> One bit that's not directly related but i decided to keep it from the 
> original patch was moving the plist-html diagnostic builder function into its 
> own file.

That is a great call indeed.

>> I'm also starting to wonder whether this diagnostics infrastructure should 
>> be in its own library, not in libAnalysis -- what do you think?
>
> Maybe! libAnalysis is currently a mess; it's basically a collection of all 
> static-analyzer-ish things that are also used outside of the static analyzer. 
> That's indeed not a good definition for a library. It incorporates a number 
> of completely unrelated things.

Not to mention my new plugins were knocked around all over the place, and 
eventually they also landed in it. I think libAnalysis is indeed well overdue 
for a revision, maybe sooner rather then later. There aren't too many other 
changes in this patch, maybe its worth doing this right away? I don't insist.


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/include/clang/Analysis/PathDiagnosticConsumers.h:36
 const Preprocessor &PP,
\
 const cross_tu::CrossTranslationUnitContext &CTU);
+#include "clang/Analysis/PathDiagnosticConsumers.def"

gribozavr wrote:
> Maybe not quite an appropriate comment for this patch, but I just realized 
> that this factory function returns void, which confused me -- where does the 
> new consumer go? Only after reading an implementation of one of these 
> functions I understood that they add the new consumer to `C`, which is a 
> vector (whose type is conveniently hidden by a typedef -- why?)
> 
> I'd suggest to make these factories more like factories, and make them return 
> a unique_ptr that the caller can put wherever they need. Of course, in a 
> separate patch.
> 
> I'd also suggest to remove the `PathDiagnosticConsumers` typedef altogether. 
> It is used just a couple of times in the code, and obfuscates code more than 
> it helps.
Will do!



Comment at: clang/lib/Analysis/TextDiagnostics.cpp:23
+namespace {
+class TextDiagnostics : public PathDiagnosticConsumer {
+  DiagnosticsEngine &Diag;

gribozavr wrote:
> "TextDiagnosticsConsumer"
> 
> Or even better, "TextPathDiagnosticsConsumer".
> 
> Same for the file name. Same for other files that define diagnostics 
> consumers.
> 
> Clarity is really important here. Very few places in the code will mention 
> this type by name, however, it is really important to distinguish this 
> diagnostics infrastructure from the rest of Clang's diagnostics.
> 
> I'm also starting to wonder whether this diagnostics infrastructure should be 
> in its own library, not in libAnalysis -- what do you think?
Renamed all the stuff.

> I'm also starting to wonder whether this diagnostics infrastructure should be 
> in its own library, not in libAnalysis -- what do you think?

Maybe! `libAnalysis` is currently a mess; it's basically a collection of all 
static-analyzer-ish things that are also used outside of the static analyzer. 
That's indeed not a good definition for a library. It incorporates a number of 
completely unrelated things.


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

https://reviews.llvm.org/D67422

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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 282798.
NoQ marked an inline comment as done.
NoQ added a reviewer: vsavchenko.
NoQ added a comment.
Herald added a subscriber: steakhal.

And rebase. Addressed comments. With @Szelethus's refactoring work the 
patch actually looks much cleaner than it used to. One bit that's not directly 
related but i decided to keep it from the original patch was moving the 
plist-html diagnostic builder function into its own file.


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

https://reviews.llvm.org/D67422

Files:
  clang/include/clang/Analysis/PathDiagnosticConsumers.def
  clang/include/clang/Analysis/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/Analyses.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
  clang/lib/Analysis/CMakeLists.txt
  clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
  clang/lib/Analysis/PlistPathDiagnosticConsumer.cpp
  clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
  clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  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
@@ -21,6 +21,7 @@
 #include "clang/Analysis/CallGraph.h"
 #include "clang/Analysis/CodeInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -30,7 +31,6 @@
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/PostOrderIterator.h"
@@ -152,7 +152,7 @@
   case PD_##NAME:  \
 CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
 default:
   llvm_unreachable("Unknown analyzer output type!");
 }
Index: clang/lib/StaticAnalyzer/Core/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Core/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -30,16 +30,13 @@
   ExprEngineCallAndReturn.cpp
   ExprEngineObjC.cpp
   FunctionSummary.cpp
-  HTMLDiagnostics.cpp
   LoopUnrolling.cpp
   LoopWidening.cpp
   MemRegion.cpp
-  PlistDiagnostics.cpp
   ProgramState.cpp
   RangeConstraintManager.cpp
   RangedConstraintManager.cpp
   RegionStore.cpp
-  SarifDiagnostics.cpp
   SimpleConstraintManager.cpp
   SimpleSValBuilder.cpp
   SMTConstraintManager.cpp
@@ -47,7 +44,6 @@
   SValBuilder.cpp
   SVals.cpp
   SymbolManager.cpp
-  TextDiagnostics.cpp
   WorkList.cpp
 
   LINK_LIBS
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -301,7 +301,7 @@
 AnalysisDiagClients Value = llvm::StringSwitch(Name)
 #define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATFN) \
   .Case(CMDFLAG, PD_##NAME)
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
   .Default(NUM_ANALYSIS_DIAG_CLIENTS);
 if (Value == NUM_ANALYSIS_DIAG_CLIENTS) {
   Diags.Report(diag::err_drv_invalid_value)
Index: clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
===
--- clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
+++ clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
@@ -18,7 +19,6 @@
 #include "clang/Lex/Preprocessor.h"
 #include "cl

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

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

Szelethus added a comment.
Herald added subscribers: ASDenysPetrov, martong.

In D67422#1667079 , @NoQ wrote:

> Casually  rename 
> `ClangDiagPathDiagConsumer` to `TextDiagnostics` according to the local 
> naming tradition.


Yoink.  You could be facing a rebasing 
nightmare here :)


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

https://reviews.llvm.org/D67422



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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Analysis/PathDiagnosticConsumers.def:23
+ANALYSIS_DIAGNOSTICS(PLIST_HTML, "plist-html", "Output analysis results using 
HTML wrapped with Plists", createPlistHTMLDiagnosticConsumer)
+ANALYSIS_DIAGNOSTICS(SARIF, "sarif", "Output analysis results in a SARIF 
file", createSarifDiagnosticConsumer)
+ANALYSIS_DIAGNOSTICS(TEXT, "text", "Text output of analysis results", 
createTextPathDiagnosticConsumer)

80 columns?



Comment at: clang/include/clang/Analysis/PathDiagnosticConsumers.h:36
 const Preprocessor &PP,
\
 const cross_tu::CrossTranslationUnitContext &CTU);
+#include "clang/Analysis/PathDiagnosticConsumers.def"

Maybe not quite an appropriate comment for this patch, but I just realized that 
this factory function returns void, which confused me -- where does the new 
consumer go? Only after reading an implementation of one of these functions I 
understood that they add the new consumer to `C`, which is a vector (whose type 
is conveniently hidden by a typedef -- why?)

I'd suggest to make these factories more like factories, and make them return a 
unique_ptr that the caller can put wherever they need. Of course, in a separate 
patch.

I'd also suggest to remove the `PathDiagnosticConsumers` typedef altogether. It 
is used just a couple of times in the code, and obfuscates code more than it 
helps.



Comment at: clang/lib/Analysis/PlistHTMLDiagnostics.cpp:27
+const cross_tu::CrossTranslationUnitContext &CTU) {
+  // Duplicate the DiagOpts object into both consumers.
+  createHTMLDiagnosticConsumer(PathDiagnosticConsumerOptions(DiagOpts), C,

Passing it by value would have made it less subtle, and probably as efficient.



Comment at: clang/lib/Analysis/TextDiagnostics.cpp:23
+namespace {
+class TextDiagnostics : public PathDiagnosticConsumer {
+  DiagnosticsEngine &Diag;

"TextDiagnosticsConsumer"

Or even better, "TextPathDiagnosticsConsumer".

Same for the file name. Same for other files that define diagnostics consumers.

Clarity is really important here. Very few places in the code will mention this 
type by name, however, it is really important to distinguish this diagnostics 
infrastructure from the rest of Clang's diagnostics.

I'm also starting to wonder whether this diagnostics infrastructure should be 
in its own library, not in libAnalysis -- what do you think?


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

https://reviews.llvm.org/D67422



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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 219829.
NoQ added a comment.
Herald added a subscriber: mgorny.

Unforget to actually move the consumer implementations to `libAnalysis`, not 
just their headers.

Casually  rename 
`ClangDiagPathDiagConsumer` to `TextDiagnostics` according to the local naming 
tradition.


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

https://reviews.llvm.org/D67422

Files:
  clang/include/clang/Analysis/PathDiagnosticConsumers.def
  clang/include/clang/Analysis/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/Analyses.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
  clang/lib/Analysis/CMakeLists.txt
  clang/lib/Analysis/HTMLDiagnostics.cpp
  clang/lib/Analysis/PlistDiagnostics.cpp
  clang/lib/Analysis/PlistHTMLDiagnostics.cpp
  clang/lib/Analysis/SarifDiagnostics.cpp
  clang/lib/Analysis/TextDiagnostics.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  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
@@ -13,6 +13,7 @@
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "ModelInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
@@ -29,7 +30,6 @@
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistration.h"
@@ -60,116 +60,6 @@
 STATISTIC(PercentReachableBlocks, "The % of reachable basic blocks.");
 STATISTIC(MaxCFGSize, "The maximum number of basic blocks in a function.");
 
-//===--===//
-// Special PathDiagnosticConsumers.
-//===--===//
-
-void ento::createPlistHTMLDiagnosticConsumer(
-PathDiagnosticConsumerOptions &&DiagOpts, PathDiagnosticConsumers &C,
-const std::string &prefix, const Preprocessor &PP,
-const cross_tu::CrossTranslationUnitContext &CTU) {
-  // Duplicate the DiagOpts object into both consumers.
-  createHTMLDiagnosticConsumer(PathDiagnosticConsumerOptions(DiagOpts), C,
-   llvm::sys::path::parent_path(prefix), PP, CTU);
-  createPlistMultiFileDiagnosticConsumer(
-  PathDiagnosticConsumerOptions(DiagOpts), C, prefix, PP, CTU);
-}
-
-namespace {
-class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer {
-  DiagnosticsEngine &Diag;
-  bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false;
-
-public:
-  ClangDiagPathDiagConsumer(PathDiagnosticConsumerOptions &&DiagOpts,
-DiagnosticsEngine &Diag)
-  : Diag(Diag), IncludePath(DiagOpts.ShouldDisplayPathNotes),
-ShouldEmitAsError(DiagOpts.ShouldDisplayWarningsAsErrors),
-FixitsAsRemarks(DiagOpts.ShouldEmitFixItHintsAsRemarks) {}
-  ~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 FlushDiagnosticsImpl(std::vector &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");
-
-auto reportPiece =
-[&](unsigned ID, SourceLocation Loc, StringRef String,
-ArrayRef Ranges, ArrayRef Fixits) {
-  if (!FixitsAsRemarks) {
-Diag.Report(Loc, ID) << String << Ranges << Fixits;
-  } else {
-Diag.Report(Loc, ID) << String << Ran

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hmm, does anybody want me to write an example tool that emits path diagnostics 
but doesn't link to `libStaticAnalyzer*`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67422



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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added inline comments.



Comment at: clang/include/clang/Analysis/PathDiagnosticConsumers.h:37
 const cross_tu::CrossTranslationUnitContext &CTU);
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
 

gribozavr wrote:
> Does this code declare e.g., createPlistDiagnosticConsumer? That function is 
> defined in libStaticAnalyzer. Declaring it libAnalysis is not appropriate, 
> and effectively introduces a layering violation. Users who link against 
> libAnalysis will get link errors if they don't link against libStaticAnalyzer.
> 
Oh crap, i forgot to move them.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67422



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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr requested changes to this revision.
gribozavr added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Analysis/PathDiagnosticConsumers.h:37
 const cross_tu::CrossTranslationUnitContext &CTU);
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
 

Does this code declare e.g., createPlistDiagnosticConsumer? That function is 
defined in libStaticAnalyzer. Declaring it libAnalysis is not appropriate, and 
effectively introduces a layering violation. Users who link against libAnalysis 
will get link errors if they don't link against libStaticAnalyzer.



Repository:
  rC Clang

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

https://reviews.llvm.org/D67422



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

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Artem Dergachev via Phabricator via cfe-commits]

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

We're finally more or less ready to allow users outside of the Static Analyzer 
to take advantage of path diagnostic consumers for emitting their warnings in 
different formats. I didn't really try to do that in practice, but most of the 
necessary APIs should be at least available now.

I'm not planning to convert Clang-Tidy to use these APIs directly because i 
understand that they're more complicated than Clang-Tidy really needs them to 
be. I'll either simplify these APIs (if they indeed can be simplified) or (more 
likely) introduce a convenient wrapper for easily building path diagnostics.

I'm not sure about renaming these classes to get rid of the "Path" prefix. 
These diagnostics aren't necessarily displaying the path (and they never did), 
but they're flexible enough for displaying paths and that's still the primary 
difference between our path diagnostics and ordinary diagnostics. Also if we 
just call them "Diagnostics" it'll be too generic and hard to distinguish from 
the Clang diagnostic engine. Suggestions are welcome :)


Repository:
  rC Clang

https://reviews.llvm.org/D67422

Files:
  clang/include/clang/Analysis/PathDiagnosticConsumers.def
  clang/include/clang/Analysis/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/Analyses.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
  clang/lib/Frontend/CompilerInvocation.cpp
  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
@@ -13,6 +13,7 @@
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "ModelInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
@@ -29,7 +30,6 @@
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistration.h"
@@ -277,7 +277,7 @@
   case PD_##NAME:  \
 CREATEFN(Opts->getDiagOpts(), PathConsumers, OutDir, PP, CTU); \
 break;
-#include "clang/StaticAnalyzer/Core/Analyses.def"
+#include "clang/Analysis/PathDiagnosticConsumers.def"
 }
   }
 }
Index: clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
@@ -11,9 +11,9 @@
 //===--===//
 
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/Version.h"
 #include "clang/Lex/Preprocessor.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/JSON.h"
Index: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/Analysis/IssueHash.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Analysis/PathDiagnosticConsumers.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/PlistSupport.h"
 #include "clang/Basic/SourceManager.h"
@@ -21,7 +22,6 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/TokenConcatenation.h"
 #include "clang/Rewrite/Core/HTMLRewrite.h"
-#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
Index: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
==