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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.
Herald added a subscriber: jdoerfert.


================
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";
----------------
Szelethus wrote:
> 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.
Hmm, in the first example we'll also have to manually reset the option to the default value if it is invalid, which is also annoying - even if easy to understand, it's also easy to forget.

And with that and a bit of polish, the lambda approach isn't really much more verbose, and definitely involves less duplication:

```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 value");
  Checker->AllowedPad = 24;
}
```
vs.
```lang=c++
auto *Checker = Mgr.registerChecker<PaddingChecker>();
Checker->AllowedPad = Mgr.getAnalyzerOptions()
        .getCheckerIntegerOption(Checker, "AllowedPad", /*Default=*/ 24,
                                 /*Validate=*/ [](int x) { return x >= 0; },
                                 /*ValidMsg=*/ "a non-negative value");
```


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

https://reviews.llvm.org/D57850