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