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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
