[PATCH] D57850: [analyzer] Emit an error rather than assert on invalid checker option input

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked 2 inline comments as done.
Szelethus added inline comments.


================
Comment at: include/clang/Basic/DiagnosticDriverKinds.td:304-305
 def err_analyzer_config_unknown : Error<"unknown analyzer-config '%0'">;
+def err_analyzer_checker_option_invalid_input : Error<
+  "invalid input for checker option '%0', that expects %1 value">;
 
----------------
NoQ wrote:
> I suggest hardcoding the word "value" in every message rather than putting it here, because it may be desirable to substitute it with something more specific or more accurate in every checker's specific circumstances.
> 
> Eg., "a non-negative integer" would be more precise than "a non-negative value".
Yup, makes total sense!


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:352-355
+  if (Checker->AllowedPad < 0)
+    Mgr.getDiagnostics().Report(diag::err_analyzer_checker_option_invalid_input)
+        << (llvm::Twine() + Checker->getTagDescription() + ":AllowedPad").str()
+        << "a non-negative";
----------------
NoQ wrote:
> 
> I passively wish for a certain amount of de-duplication that wouldn't require every checker to obtain a diagnostics engine every time it tries to read an option. Eg.,
> ```lang=c++
>   auto *Checker = Mgr.registerChecker<PaddingChecker>();
>   Checker->AllowedPad = Mgr.getAnalyzerOptions()
>           .getCheckerIntegerOption(Checker, "AllowedPad", 24);
>   if (Checker->AllowedPad < 0)
>     Mgr.reportInvalidOptionValue(Checker, "AllowedPad", "a non-negative");
> ```
> 
> Or maybe even something like that:
> 
> ```lang=c++
>   auto *Checker = Mgr.registerChecker<PaddingChecker>();
>   Checker->AllowedPad = Mgr.getAnalyzerOptions()
>           .getCheckerIntegerOption(Checker, "AllowedPad", 24,
>                   [](int x) -> Option<std::string> {
>                       if (x < 0) {
>                         // Makes getCheckerIntegerOption() emit a diagnostic
>                         // and return the default value.
>                         return "a non-negative";
>                       }
>                       // Makes getCheckerIntegerOption() successfully return
>                       // the user-specified value.
>                       return None;
>                   });
> ```
> I.e., a validator lambda.
First one, sure. I'm a little unsure about the second: No other "options"-like classes have access to a `DiagnosticsEngine` in clang, as far as I'm aware, and I guess keeping `AnalyzerOptions` as simple is possible is preferred. Not only that, but a validator lambda seems an to be an overkill (though really-really cool) solution. Your first bit of code is far more readable IMO.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57850