[PATCH] D53483: [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Overall looks good if the community agrees with the directions. Some comments inline.



================
Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:243
+  /// specified.
+  StringRef getStringOption(StringRef Name, StringRef DefaultVal);
 
----------------
If you want the devs to maintain an explicit getter for each analyzer option rather than making this one public at some point, please document expectation this somewhere.


================
Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:157
+                     .getAsInteger(10, Ret);
+  assert(!HasFailed && "analyzer-config option should be numeric");
+  (void)HasFailed;
----------------
Can this assert be triggered using a bad invocation of the analyzer? I wonder if it is a good idea to use asserts to validate user input. Maybe it would be better to generate a warning and return the default value?


================
Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:205
+                     .getAsInteger(10, Ret);
+  assert(!HasFailed && "analyzer-config option should be numeric");
+  (void)HasFailed;
----------------
Same as above.


Repository:
  rC Clang

https://reviews.llvm.org/D53483