[PATCH] D57855: [analyzer] Reimplement checker options

[Balogh, Ádám via Phabricator via cfe-commits]

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.


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

https://reviews.llvm.org/D57855