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