[PATCH] D77085: [clang-tidy] Added support for validating configuration options

[Nathan James via Phabricator via cfe-commits]

njames93 marked 8 inline comments as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:25
+  llvm::raw_svector_ostream Output(Buffer);
+  Output << "warning: Option not found '" << OptionName << "'\n";
+  return std::string(Buffer);
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > I think the diagnostic text should probably not start with a capital letter. Also, the name of the classes are confusing in that they say error but the diagnostic is a warning. When I hear "error", the class name makes me think this would stop compilation and give a nonzero result code from the program.
> Sorry, I was unclear with what I was looking for.
> 
> It looks to me like the behavior of `OptionError` is to act more like a warning than an error in that a message is printed and execution of the tool continues, correct? If so, then I'd prefer to rename it to `OptionWarning` (unless you want to generalize it to be either a warning or an error with a return code, then `OptionDiagnostic` would be good), and all the subclasses to be `FooWarning`. Or did I misunderstand the design?
> 
> Also, I like having the `warning:` in the message (though we could bikeshed if we want to make it look less like a check warning by doing something like `warning [clang-tidy command line]:`), but maybe that's being added by `OptionsView::logErrToStdErr()`?
`OptionError` is just there for convenience to avoid having to retype the `log` and `convertToErrorCode` function. Arguably, the `MissingOptionError` isn't really an error, but the `UnparseableEnumOptionError` and `UnparseableIntegerOptionError` should be classed as errors.

>From what it seems with other errors, the message function shouldn't contain a `warning:` or `error:` prefix, instead that now gets added in `OptionsView::logErrToStdErr`. The output for enumerations looks like 
```
warning: invalid configuration value 'camelback' for option 'readability-identifier-naming.FunctionCase'; did you mean 'camelBack'?```

I do think down the line this should be actually handled as a `ClangTidyDiagnostic` however that is a much larger change to implement given how diagnostics are currently handled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77085