Szelethus marked an inline comment as done.
Szelethus added inline comments.
Herald added a subscriber: Charusso.
================
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:
> 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?
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
