[PATCH] D57855: [analyzer] Reimplement checker options

Balogh, Ádám via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 11 07:47:22 PDT 2019

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

Comment at: include/clang/StaticAnalyzer/Checkers/CheckerBase.td:49
+  string              PackageName = name;
+  list<CmdLineOption> PackageOptions;
+  Package             ParentPackage;
Please add a comment that this field is optional.

Comment at: include/clang/StaticAnalyzer/Checkers/CheckerBase.td:87
+  string              HelpText;
+  list<CmdLineOption> CheckerOptions;
+  list<Checker>       Dependencies;
Same as above.

Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:156
+      return FullName == Rhs.FullName;
+    }
Are we able now to distinguish checkers of the same short names but in different packages? In earlier versions the short name had to be different to avoid conflicts.

Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:282
+  /// \c Checkers field. For convenience purposes.
+  llvm::StringMap<size_t> PackageSizes;
Cannot we store the size of the `Package` in the `Package` itself?

Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:50
+  }
Could we maybe use `std::less<CheckerOrPackageInfo>`?

Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:68
+                        CheckerOrPackageFullNameLT{}) &&
+         "Can't do binary search on a non-sorted collection!");
I would better keep the original message here. It has a more formal style.

Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:181
+  llvm::sort(Checkers, CheckerNameLT{});
+  llvm::sort(Packages, PackageNameLT{});
We always have packages first. Please swap these lines.

Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:307
+  assert(CheckerIt != Checkers.end() &&
+         "Failed to find the checker while attempting to set up it's "
+         "dependencies!");
Typo: it's -> its.

Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:371
+    // We can't get away with binaryFind here, as it uses lower_bound.
+    auto CheckerIt = llvm::find(Checkers, CheckerInfo(SuppliedChecker));
+    if (CheckerIt != Checkers.end())
Please explain the problem with `std::lower_bound()`. I do not see why we cannot use it (followed by a check whether we found an element equal to the searched element, not a greater one). I cannot see any excuse to use linear search instead of binary one in a sorted collection.



More information about the cfe-commits mailing list