[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-06 Thread Raphael Isemann via Phabricator via cfe-commits 
teemperor added a comment.

Fixed here to get the bot running again: 
https://github.com/llvm/llvm-project/commit/7308e1432624f02d4e652ffa70e40d0eaa89fdb3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-06 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus added a comment.

In D82585#2133260 , @teemperor wrote:

> Fixed here to get the bot running again: 
> https://github.com/llvm/llvm-project/commit/7308e1432624f02d4e652ffa70e40d0eaa89fdb3


Thank you so much! Kind of ironic considering that this entire patch was all 
about fixing them in the first place :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-06 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus added a comment.

I'm on it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-06 Thread Raphael Isemann via Phabricator via cfe-commits 
teemperor added a comment.

This broke the Green Dragon build for Clang: 
http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/

  FAILED: bin/diagtool 
  : && /Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/bin/clang++ 
 -Wdocumentation -fPIC -fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -fmodules 
-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/module.cache
 -fcxx-modules -Xclang -fmodules-local-submodule-visibility -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -fno-common -Woverloaded-virtual 
-Wno-nested-anon-types -O3  -isysroot 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk
 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -Wl,-dead_strip 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/diagtool_main.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/DiagTool.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/DiagnosticNames.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/FindDiagnosticID.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/ListWarnings.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/ShowEnabledWarnings.cpp.o 
tools/clang/tools/diagtool/CMakeFiles/diagtool.dir/TreeView.cpp.o  -o 
bin/diagtool  -Wl,-rpath,@loader_path/../lib  lib/libLLVMSupport.a  
lib/libclangBasic.a  lib/libclangFrontend.a  lib/libclangDriver.a  
lib/libclangParse.a  lib/libclangSerialization.a  lib/libclangSema.a  
lib/libclangEdit.a  lib/libclangAnalysis.a  lib/libclangASTMatchers.a  
lib/libclangAST.a  lib/libclangLex.a  lib/libclangBasic.a  
lib/libLLVMFrontendOpenMP.a  lib/libLLVMTransformUtils.a  lib/libLLVMAnalysis.a 
 lib/libLLVMObject.a  lib/libLLVMMCParser.a  lib/libLLVMMC.a  
lib/libLLVMDebugInfoCodeView.a  lib/libLLVMDebugInfoMSF.a  lib/libLLVMTextAPI.a 
 lib/libLLVMBitReader.a  lib/libLLVMOption.a  lib/libLLVMProfileData.a  
lib/libLLVMCore.a  lib/libLLVMBinaryFormat.a  lib/libLLVMRemarks.a  
lib/libLLVMBitstreamReader.a  lib/libLLVMSupport.a  -lz  -lcurses  -lm  
lib/libLLVMDemangle.a && :
  Undefined symbols for architecture x86_64:
"clang::ento::PackageInfo::dumpToStream(llvm::raw_ostream&) const", 
referenced from:
clang::ento::PackageInfo::dump() const in ShowEnabledWarnings.cpp.o
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(CompilerInstance.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(CompilerInvocation.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(CreateInvocationFromCommandLine.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(TextDiagnosticBuffer.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(FrontendAction.cpp.o)
clang::ento::PackageInfo::dump() const in 
libclangFrontend.a(FrontendOptions.cpp.o)
...
"clang::ento::CmdLineOption::dumpToStream(llvm::raw_ostream&) const", 
referenced from:
clang::ento::CmdLineOption::dump() const in ShowEnabledWarnings.cpp.o
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(CompilerInstance.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(CompilerInvocation.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(CreateInvocationFromCommandLine.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(TextDiagnosticBuffer.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(FrontendAction.cpp.o)
clang::ento::CmdLineOption::dump() const in 
libclangFrontend.a(FrontendOptions.cpp.o)
...
"clang::ento::CheckerInfo::dumpToStream(llvm::raw_ostream&) const", 
referenced from:
clang::ento::CheckerInfo::dump() const in ShowEnabledWarnings.cpp.o
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(CompilerInstance.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(CompilerInvocation.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(CreateInvocationFromCommandLine.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(TextDiagnosticBuffer.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(FrontendAction.cpp.o)
clang::ento::CheckerInfo::dump() const in 
libclangFrontend.a(FrontendOptions.cpp.o)
...
  ld: symbol(s) not found for architecture x86_64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-04 Thread Kristóf Umann via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb6cbe6cb0399: [analyzer][NFC] Move the data structures from 
CheckerRegistry to the Core… (authored by Szelethus).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585

Files:
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h
  clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/CheckerRegistryData.cpp
  clang/lib/StaticAnalyzer/Core/Environment.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalyzerHelpFlags.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -11,6 +11,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerRegistryData.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
@@ -100,7 +101,7 @@
 llvm::raw_svector_ostream OS(Buf);
 C.getAnalysisManager()
 .getCheckerManager()
-->getCheckerRegistry()
+->getCheckerRegistryData()
 .printEnabledCheckerList(OS);
 // Strip a newline off.
 auto R =
Index: clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
@@ -23,11 +23,11 @@
 ArrayRef> checkerRegistrationFns)
 : Context(), LangOpts(Context.getLangOpts()), AOptions(AOptions),
   PP(), Diags(Context.getDiagnostics()),
-  Registry(
-  std::make_unique(plugins, Context.getDiagnostics(),
-AOptions, checkerRegistrationFns)) {
-  Registry->initializeRegistry(*this);
-  Registry->initializeManager(*this);
+  RegistryData(std::make_unique()) {
+  CheckerRegistry Registry(*RegistryData, plugins, Context.getDiagnostics(),
+   AOptions, checkerRegistrationFns);
+  Registry.initializeRegistry(*this);
+  Registry.initializeManager(*this);
   finishedCheckerRegistration();
 }
 
@@ -36,8 +36,9 @@
DiagnosticsEngine ,
ArrayRef plugins)
 : LangOpts(LangOpts), AOptions(AOptions), Diags(Diags),
-  Registry(std::make_unique(plugins, Diags, AOptions)) {
-  Registry->initializeRegistry(*this);
+  RegistryData(std::make_unique()) {
+  CheckerRegistry Registry(*RegistryData, plugins, Diags, AOptions, {});
+  Registry.initializeRegistry(*this);
 }
 
 CheckerManager::~CheckerManager() {
Index: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -25,14 +25,13 @@
 
 using namespace clang;
 using namespace ento;
+using namespace checker_registry;
 using llvm::sys::DynamicLibrary;
 
 //===--===//
 // Utilities.
 //===--===//
 
-using RegisterCheckersFn = void (*)(CheckerRegistry &);
-
 static bool isCompatibleAPIVersion(const char *VersionString) {
   // If the version string is null, its not an analyzer plugin.
   if (!VersionString)
@@ -43,140 +42,17 @@
   return strcmp(VersionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
-namespace {
-template  struct FullNameLT {
-  bool operator()(const T , const T ) {
-return Lhs.FullName < Rhs.FullName;
-  }
-};
-
-using PackageNameLT = FullNameLT;
-using CheckerNameLT = FullNameLT;
-} // end of anonymous namespace
-
-template 
-static std::conditional_t::value,
-  typename CheckerOrPackageInfoList::const_iterator,
-  typename CheckerOrPackageInfoList::iterator>
-binaryFind(CheckerOrPackageInfoList , StringRef FullName) {
-
-  using CheckerOrPackage = typename CheckerOrPackageInfoList::value_type;
-  using CheckerOrPackageFullNameLT = FullNameLT;
-
-  assert(llvm::is_sorted(Collection, CheckerOrPackageFullNameLT{}) &&
- "In order to efficiently gather

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-03 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware accepted this revision.
baloghadamsoftware added a comment.
This revision is now accepted and ready to land.

It looks OK.


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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-03 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h:216
+  /// This output is not intended to be machine-parseable.
+  void printCheckerWithDescList(const AnalyzerOptions , raw_ostream 
,
+size_t MaxNameChars = 30) const;

baloghadamsoftware wrote:
> Why not store `AnOpts` in `CheckerRegistryData`? It would save the need to 
> pass it to every function separately.
This is definitely a point where I chose a well defined interface over 
convenience. These print functions only have a single user, `CompilerInstance`, 
which must juggle all sorts of analyzer managers anyways, so it wouldn't have 
made the code any more readable in my opinion.

The idea was to constrain all data this class has to be strictly about what was 
processed by `CheckerRegistry`. While mildly inconvenient, the only way to 
retrieve this object would be from `CheckerManager`, that also has a getter to 
`AnalyzerOptions`, so in case someone were to use this for debug purposes or 
something, they certainly can.


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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-03 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus updated this revision to Diff 275378.
Szelethus marked 6 inline comments as done.
Szelethus added a comment.

Small fixes according to reviewer comments.


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

https://reviews.llvm.org/D82585

Files:
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h
  clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/CheckerRegistryData.cpp
  clang/lib/StaticAnalyzer/Core/Environment.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalyzerHelpFlags.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -11,6 +11,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerRegistryData.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
@@ -100,7 +101,7 @@
 llvm::raw_svector_ostream OS(Buf);
 C.getAnalysisManager()
 .getCheckerManager()
-->getCheckerRegistry()
+->getCheckerRegistryData()
 .printEnabledCheckerList(OS);
 // Strip a newline off.
 auto R =
Index: clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
@@ -23,11 +23,11 @@
 ArrayRef> checkerRegistrationFns)
 : Context(), LangOpts(Context.getLangOpts()), AOptions(AOptions),
   PP(), Diags(Context.getDiagnostics()),
-  Registry(
-  std::make_unique(plugins, Context.getDiagnostics(),
-AOptions, checkerRegistrationFns)) {
-  Registry->initializeRegistry(*this);
-  Registry->initializeManager(*this);
+  RegistryData(std::make_unique()) {
+  CheckerRegistry Registry(*RegistryData, plugins, Context.getDiagnostics(),
+   AOptions, checkerRegistrationFns);
+  Registry.initializeRegistry(*this);
+  Registry.initializeManager(*this);
   finishedCheckerRegistration();
 }
 
@@ -36,8 +36,9 @@
DiagnosticsEngine ,
ArrayRef plugins)
 : LangOpts(LangOpts), AOptions(AOptions), Diags(Diags),
-  Registry(std::make_unique(plugins, Diags, AOptions)) {
-  Registry->initializeRegistry(*this);
+  RegistryData(std::make_unique()) {
+  CheckerRegistry Registry(*RegistryData, plugins, Diags, AOptions, {});
+  Registry.initializeRegistry(*this);
 }
 
 CheckerManager::~CheckerManager() {
Index: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -25,14 +25,13 @@
 
 using namespace clang;
 using namespace ento;
+using namespace checker_registry;
 using llvm::sys::DynamicLibrary;
 
 //===--===//
 // Utilities.
 //===--===//
 
-using RegisterCheckersFn = void (*)(CheckerRegistry &);
-
 static bool isCompatibleAPIVersion(const char *VersionString) {
   // If the version string is null, its not an analyzer plugin.
   if (!VersionString)
@@ -43,140 +42,17 @@
   return strcmp(VersionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
-namespace {
-template  struct FullNameLT {
-  bool operator()(const T , const T ) {
-return Lhs.FullName < Rhs.FullName;
-  }
-};
-
-using PackageNameLT = FullNameLT;
-using CheckerNameLT = FullNameLT;
-} // end of anonymous namespace
-
-template 
-static std::conditional_t::value,
-  typename CheckerOrPackageInfoList::const_iterator,
-  typename CheckerOrPackageInfoList::iterator>
-binaryFind(CheckerOrPackageInfoList , StringRef FullName) {
-
-  using CheckerOrPackage = typename CheckerOrPackageInfoList::value_type;
-  using CheckerOrPackageFullNameLT = FullNameLT;
-
-  assert(llvm::is_sorted(Collection, CheckerOrPackageFullNameLT{}) &&
- "In order to efficiently gather checkers/packages, this function "
- "expects them to be already sorted!");
-
-

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-03 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h:216
+  /// This output is not intended to be machine-parseable.
+  void printCheckerWithDescList(const AnalyzerOptions , raw_ostream 
,
+size_t MaxNameChars = 30) const;

Why not store `AnOpts` in `CheckerRegistryData`? It would save the need to pass 
it to every function separately.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerRegistryData.cpp:166
+void CheckerRegistryData::printEnabledCheckerList(const AnalyzerOptions 
,
+  raw_ostream ) const {
+  for (const auto *i : EnabledCheckers)

Why do we need the new `AnOpts` parameter here? It does not seem to be used at 
all.


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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-03 Thread Balázs Kéri via Phabricator via cfe-commits 
balazske added a comment.

Basically this looks good in the current way too, but I can only assume that 
the code was copied correctly. A rule could be that the `CheckerRegistryData` 
is manipulated only by `CheckerRegistry` but get or print functions can be in 
`CheckerRegistryData`.




Comment at: clang/unittests/StaticAnalyzer/CallEventTest.cpp:13
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"

This line seems to be not needed here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus marked 6 inline comments as done.
Szelethus added a comment.

In D82585#2122634 , @balazske wrote:

> It looks like that the dependency manipulation and computation functions


I guess there are two ways of looking at this:

- CheckerRegistry only contains the logic CheckerRegistryData cannot due to the 
library layout, but it is otherwise minimal
- CheckerRegistryData is data only, and all logic is placed in CheckerRegistry

I would prefer the second option -- dependency resolving is one of the most 
crucial, and very non-trivial task of `CheckerRegistry`, and while moving it to 
`CheckerRegistryData` wouldn't violate the library layout, it would certainly 
go outside of what I'd expect a data-centric class to do. I also would like to 
keep the interface of it minimal.

> and the checker and option add functions can be member of 
> `CheckerRegistryData`. It would be a bit more simple (no `Data.` call), and 
> these belong naturally to there.

I don't want to encourage the modification of the data by anyone other then 
CheckerRegistry.

On another note, this would break all plugin code. We haven't shied away from 
that in the past, but historically speaking, plugins and the unit test files 
only interacted with CheckerRegistry, and seeing how it is the logic behind 
checker registration, I think its fitting.


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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-30 Thread Balázs Kéri via Phabricator via cfe-commits 
balazske added a comment.

It looks like that the dependency manipulation and computation functions, and 
the checker and option add functions can be member of `CheckerRegistryData`. It 
would be a bit more simple (no `Data.` call), and these belong naturally to 
there.




Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:155
   /// Constructs a CheckerManager without requiring an AST. No checker
-  /// registration will take place. Only useful for retrieving the
-  /// CheckerRegistry and print for help flags where the AST is unavalaible.
+  /// registration will take placer. Only useful when one needs to print the
+  /// help flags through CheckerRegistryData, and the AST is unavalaible.

placer -> place



Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h:199
+  PackageInfoList Packages;
+  /// Used for couting how many checkers belong to a certain package in the
+  /// \c Checkers field. For convenience purposes.

couting -> counting


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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus updated this revision to Diff 274445.
Szelethus added a comment.

Fixes according to reviewer comments!

edit: from @gamesh411.


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

https://reviews.llvm.org/D82585

Files:
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h
  clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/CheckerRegistryData.cpp
  clang/lib/StaticAnalyzer/Core/Environment.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalyzerHelpFlags.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
  clang/unittests/StaticAnalyzer/CallEventTest.cpp
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -11,6 +11,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerRegistryData.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
@@ -100,8 +101,9 @@
 llvm::raw_svector_ostream OS(Buf);
 C.getAnalysisManager()
 .getCheckerManager()
-->getCheckerRegistry()
-.printEnabledCheckerList(OS);
+->getCheckerRegistryData()
+.printEnabledCheckerList(C.getAnalysisManager().getAnalyzerOptions(),
+ OS);
 // Strip a newline off.
 auto R =
 std::make_unique(*BT, OS.str().drop_back(1), N);
Index: clang/unittests/StaticAnalyzer/CallEventTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallEventTest.cpp
+++ clang/unittests/StaticAnalyzer/CallEventTest.cpp
@@ -10,6 +10,7 @@
 #include "CheckerRegistration.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
Index: clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
@@ -23,11 +23,11 @@
 ArrayRef> checkerRegistrationFns)
 : Context(), LangOpts(Context.getLangOpts()), AOptions(AOptions),
   PP(), Diags(Context.getDiagnostics()),
-  Registry(
-  std::make_unique(plugins, Context.getDiagnostics(),
-AOptions, checkerRegistrationFns)) {
-  Registry->initializeRegistry(*this);
-  Registry->initializeManager(*this);
+  RegistryData(std::make_unique()) {
+  CheckerRegistry Registry(*RegistryData, plugins, Context.getDiagnostics(),
+   AOptions, checkerRegistrationFns);
+  Registry.initializeRegistry(*this);
+  Registry.initializeManager(*this);
   finishedCheckerRegistration();
 }
 
@@ -36,8 +36,9 @@
DiagnosticsEngine ,
ArrayRef plugins)
 : LangOpts(LangOpts), AOptions(AOptions), Diags(Diags),
-  Registry(std::make_unique(plugins, Diags, AOptions)) {
-  Registry->initializeRegistry(*this);
+  RegistryData(std::make_unique()) {
+  CheckerRegistry Registry(*RegistryData, plugins, Diags, AOptions, {});
+  Registry.initializeRegistry(*this);
 }
 
 CheckerManager::~CheckerManager() {
Index: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -25,14 +25,13 @@
 
 using namespace clang;
 using namespace ento;
+using namespace checker_registry;
 using llvm::sys::DynamicLibrary;
 
 //===--===//
 // Utilities.
 //===--===//
 
-using RegisterCheckersFn = void (*)(CheckerRegistry &);
-
 static bool isCompatibleAPIVersion(const char *VersionString) {
   // If the version string is null, its not an analyzer plugin.
   if (!VersionString)
@@ -43,140 +42,17 @@
   return strcmp(VersionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
-namespace {

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus marked 2 inline comments as done.
Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h:83
+
+using CmdLineOptionList = llvm::SmallVector;
+

gamesh411 wrote:
> Could you please explain why use zero for the small-vector element count? My 
> first thought would be that literally anything other than 0 would be more 
> beneficial because of non-dynamic storage, even if we have use a totally 
> ad-hoc value here (like 8 or 42 or whatever). Why not std::vector if we do 
> not want static storage? Maybe there is something that I'm just not aware of 
> :D
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h

> `SmallVector` has grown a few other minor advantages over `std::vector`, 
> causing `SmallVector` to be preferred over `std::vector`.



Comment at: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:472
 
-  using CmdLineOption = CheckerRegistry::CmdLineOption;
+  using CmdLineOption = CmdLineOption;
 

gamesh411 wrote:
> A bit confused here. It is not immediately clear for me from which namespace 
> are we importing CmdLineOption. Is it possible to use a fully qualified type 
> name on the RHS of the using statement?
This looks stupid because it is :^) Accidentally left it there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-30 Thread Endre Fülöp via Phabricator via cfe-commits 
gamesh411 added a comment.

Good job, massive props to you for such an overhaul.




Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:155
   /// Constructs a CheckerManager without requiring an AST. No checker
-  /// registration will take place. Only useful for retrieving the
-  /// CheckerRegistry and print for help flags where the AST is unavalaible.
+  /// registration will take placer. Only useful when one needs to print the
+  /// help flags through CheckerRegistryData, and the AST is unavalaible.

placer -> place



Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h:16
+// this information, such as enforcing rules on checker dependency bug 
emission,
+// ensuring all checker options were querried, etc.
+//

querried -> queried



Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h:83
+
+using CmdLineOptionList = llvm::SmallVector;
+

Could you please explain why use zero for the small-vector element count? My 
first thought would be that literally anything other than 0 would be more 
beneficial because of non-dynamic storage, even if we have use a totally ad-hoc 
value here (like 8 or 42 or whatever). Why not std::vector if we do not want 
static storage? Maybe there is something that I'm just not aware of :D



Comment at: clang/lib/StaticAnalyzer/Core/CheckerRegistryData.cpp:179
+
+  // Its usually ill-advised to use multimap, but clang will terminate after
+  // this function.

Its -> It's



Comment at: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:472
 
-  using CmdLineOption = CheckerRegistry::CmdLineOption;
+  using CmdLineOption = CmdLineOption;
 

A bit confused here. It is not immediately clear for me from which namespace 
are we importing CmdLineOption. Is it possible to use a fully qualified type 
name on the RHS of the using statement?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82585



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

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-25 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, dcoughlin, vsavchenko, martong, 
balazske, baloghadamsoftware.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, 
gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, mgrang, rnkovacs, 
szepet, whisperity, mgorny.

If you were around the analyzer for a while now, you must've seen a lot of 
patches that awkwardly puts code from one library to the other:

- D75360  moves the constructors of 
CheckerManager, which lies in the Core library, to the Frontend library. Most 
the patch itself was a struggle along the library lines.
- D78126  had to be reverted because 
dependency information would be utilized in the Core library, but the actual 
data lied in the frontend. D78126#inline-751477 
 touches on this issue as well.

This stems from the often mentioned problem: the Frontend library depends on 
Core and Checkers, Checkers depends on Core. The checker registry functions 
(`registerMallocChecker`, etc) lie in the Checkers library in order to keep 
each checker its own module. What this implies is that checker registration 
cannot take place in the Core, but the Core might still want to use the data 
that results from it (which checker/package is enabled, dependencies, etc).

D54436  was the patch that initiated this. 
Back in the days when CheckerRegistry was super dumb and buggy, it implemented 
a non-documented solution to this problem by keeping the data in the Core, and 
leaving the logic in the Frontend. At the time when the patch landed, the 
merger to the Frontend made sense, because the data hadn't been utilized 
anywhere, and the whole workaround without any documentation made little sense 
to me.

So, lets put the data back where it belongs, in the Core library.

Side note: I can't help but cringe at the fact how ridiculously awkward the 
library lines are. I feel like I'm thinking too much inside the box, but I 
guess this is just the price of keeping the checkers so modularized.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82585

Files:
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h
  clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/CheckerRegistryData.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalyzerHelpFlags.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
  clang/unittests/StaticAnalyzer/CallEventTest.cpp
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -100,8 +100,9 @@
 llvm::raw_svector_ostream OS(Buf);
 C.getAnalysisManager()
 .getCheckerManager()
-->getCheckerRegistry()
-.printEnabledCheckerList(OS);
+->getCheckerRegistryData()
+.printEnabledCheckerList(C.getAnalysisManager().getAnalyzerOptions(),
+ OS);
 // Strip a newline off.
 auto R =
 std::make_unique(*BT, OS.str().drop_back(1), N);
Index: clang/unittests/StaticAnalyzer/CallEventTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallEventTest.cpp
+++ clang/unittests/StaticAnalyzer/CallEventTest.cpp
@@ -10,6 +10,7 @@
 #include "CheckerRegistration.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
Index: clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
+++ clang/lib/StaticAnalyzer/Frontend/CreateCheckerManager.cpp
@@ -23,11 +23,11 @@
 ArrayRef> checkerRegistrationFns)
 : Context(), LangOpts(Context.getLangOpts()), AOptions(AOptions),
   PP(), Diags(Context.getDiagnostics()),
-  Registry(
-  std::make_unique(plugins, Context.getDiagnostics(),
-AOptions, checkerRegistrationFns)) {
-  Registry->initializeRegistry(*this);
-  Registry->initializeManager(*this);
+  RegistryData(std::make_unique()) {
+  CheckerRegistry Registry(*RegistryData, plugins, Context.getDiagnostics(),
+