[PATCH] D57855: [analyzer] Reimplement checker options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.
Herald added a subscriber: jdoerfert.


================
Comment at: include/clang/StaticAnalyzer/Checkers/CheckerBase.td:13-14
 
+/// Describes a checker or package option type. This is important for validating
+/// user supplied inputs.
+class CmdLineOptionTypeEnum<bits<2> val> {
----------------
Might want to add a comment mentioning that additional enumerators require changes to the tablegen code.


================
Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:166
+
+    PackageInfo(StringRef FullName) : FullName(FullName) {}
+  };
----------------
Make this `explicit`?


================
Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:248-250
+  void printCheckerWithDescList(raw_ostream &out,
+                                size_t maxNameChars = 30) const;
+  void printEnabledCheckerList(raw_ostream &out) const;
----------------
If we're changing these, can you fix up the parameter names for our coding conventions?


================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:295
+
+  SameFullName(StringRef RequestedName) : RequestedName(RequestedName) {}
+
----------------
`explicit`?


================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:306
+
+void CheckerRegistry::addDependency(StringRef fullName, StringRef dependency) {
+  auto CheckerIt = llvm::find_if(Checkers, SameCheckerName(fullName));
----------------
Naming conventions.


================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:349
+
+  PackageIt->CmdLineOptions.push_back(
+      {OptionType, OptionName, DefaultValStr, Description});
----------------
`emplace_back()` instead?


================
Comment at: utils/TableGen/ClangSACheckersEmitter.cpp:95
+  if (BitsInit *BI = R.getValueAsBitsInit("Type")) {
+    switch(getValueFromBitsInit(BI, R)) {
+      case 0:
----------------
There should be come comments here marrying this up with the .td file.

Also, the formatting here looks somewhat off (the case statement seem to be indented too far).


================
Comment at: utils/TableGen/ClangSACheckersEmitter.cpp:171
+        "#ifdef GET_PACKAGE_OPTIONS\n";
+  for (const Record *package : packages) {
+
----------------
Naming conventions.


================
Comment at: utils/TableGen/ClangSACheckersEmitter.cpp:253
+        "#ifdef GET_CHECKER_OPTIONS\n";
+  for (const Record *checker : checkers) {
+
----------------
Naming conventions.


================
Comment at: utils/TableGen/ClangSACheckersEmitter.cpp:258
+
+
+    std::vector<Record *> CheckerOptions = checker
----------------
Can remove some of the extra vertical whitespace.


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

https://reviews.llvm.org/D57855