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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Looks cool, thanks!!



================
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">;
 
----------------
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".


================
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";
----------------

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57850