NoQ added a comment.
> Would you mind me committing as is?
Np, please do, the patch is great regardless!
================
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:
> > 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");
> > ```
> Alright, so I've given this a lot of thought, here's where I'm standing on
> the issue:
>
> * I would prefer not to add `DiagnosticsEngine` to `AnalyzerOptions`. In
> fact, I'd prefer not to add it even as a parameter to one of it's methods --
> designwise, it should be a simple mapping of the command line parameters, not
> doing any complicated hackery.
> * You got me convinced the validator lambda thing ;). However, a nice
> implementation of this (emphasis on //nice//) is most definitely a bigger
> undertaking.
> * Once we're at the topic of "easy to forget", we could also verify
> compile-time whether checker options are actually used -- what I'm thinking
> here, is something like this:
>
> ```
> auto *Checker = Mgr.registerChecker<PaddingChecker>();
> Mgr.initFieldWithOption(Checker, "AllowedPad",
> // Note that we should be able
> // to know the default value.
> Checker->AllowedPad,
> // We could make this optional by defining a
> // default validator...
> /*Validate=*/ [](int x) { return x >= 0; },
> // ...aaaand a default error message.
> /*ValidMsg=*/ "a non-negative value");
> ```
> `CheckerManager` later (once all checker registry functions finished) could
> validate, with the help of `CheckerRegistry`, whether
> * All options for a given checker were queried for,
> * The supplied checker options is valid, if not, restore them in
> compatibility mode, emit an error otherwise,
> * No list is complete without a third item.
>
> For now, I admit, I have little interest in this. Would you mind me
> committing as is?
> Once we're at the topic of "easy to forget", we could also verify
> compile-time whether checker options are actually used -- what I'm thinking
> here, is something like this:
> ```lang=c++
> auto *Checker = Mgr.registerChecker<PaddingChecker>();
> Mgr.initFieldWithOption(Checker, "AllowedPad",
> // Note that we should be able
> // to know the default value.
> Checker->AllowedPad,
> // We could make this optional by defining a
> // default validator...
> /*Validate=*/ [](int x) { return x >= 0; },
> // ...aaaand a default error message.
> /*ValidMsg=*/ "a non-negative value");
> ```
For **ULTIMATE** reliability and reusability, i'd also use a pointer-to-member
argument instead of a pass-by-reference out-parameter, so that make sure that
the value is indeed stored in the checker:
```lang=c++
Mgr.initFieldWithOption(Checker, "AllowedPad",
&PaddingChecker::AllowedPad,
...)
```
(sorry, couldn't help myself)
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
