This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Added support for validating configuration options
ClosedPublic

Authored by njames93 on Mar 30 2020, 1:20 PM.

Details

Summary

Adds support for ClangTidyCheck::OptionsView to deteremine:

  • If an option is found in the configuration.
  • If an integer option read from configuration is parsable to an integer.
  • Parse and Serialize enum configuration options directly using a mapping from llvm::StringRef to EnumType.
  • If an integer or enum option isn't parseable but there is a default value it will issue a warning to stderr that the config value hasn't been used.
  • If an enum option isn't parsable it can provide a hint if the value was a typo.

Diff Detail

Event Timeline

njames93 created this revision.Mar 30 2020, 1:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2020, 1:20 PM

This pretty much nulls out (clang-tidy) Warn on invalid "case" configurations for readability-identifier-naming I have moved that into a follow up patch that can be submitted now if you'd like.

njames93 added a project: Restricted Project.Mar 30 2020, 1:25 PM
njames93 updated this revision to Diff 253751.Mar 30 2020, 5:46 PM
  • Fix assertions failing
njames93 updated this revision to Diff 254552.Apr 2 2020, 9:50 AM
  • Small refactor

Thank you for working on this, I think it's going to be a very useful interface!

clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
26

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.

109–114

Elide braces

123–125

Preference to add the braces to this case because the surrounding ones do as well.

clang-tools-extra/clang-tidy/ClangTidyCheck.h
41

Make the constructor explicit? (May want to consider the same for the other ctors, but this one seems more dangerous.)

205

Don't use auto as the type is not spelled out.

209

Default.str() instead?

227–231

Same here as above.

288

Elide braces

njames93 updated this revision to Diff 254890.Apr 3 2020, 12:48 PM
  • Address nits
aaron.ballman added inline comments.Apr 3 2020, 1:09 PM
clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
26

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()?

njames93 marked 8 inline comments as done.Apr 3 2020, 3:12 PM
njames93 added inline comments.
clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
26

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.

njames93 updated this revision to Diff 255023.Apr 4 2020, 4:14 AM
  • Extended support for boolean types

I added support for bools. The previous integer parsing behaviour is still there, however now it also responds to true or false.
This won't parse True|TRUE|False|FALSE etc as I wanted it to be in line with .clang-format configuration files for handling of bool.

aaron.ballman accepted this revision.Apr 7 2020, 11:19 AM

LGTM with a few minor nits.

clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
119

Elide braces

146

Elide braces

This revision is now accepted and ready to land.Apr 7 2020, 11:19 AM
This revision was automatically updated to reflect the committed changes.

A recent commit has taken down a whole bunch of bots. The build error messages all seem to point to code in this patch. If this is indeed the cause, please revert.

A recent commit has taken down a whole bunch of bots. The build error messages all seem to point to code in this patch. If this is indeed the cause, please revert.

I was aware and hopefully this fixes the issue https://github.com/llvm/llvm-project/commit/0361798dbeb6ead0a79ab7985f02da347fce988e

A recent commit has taken down a whole bunch of bots. The build error messages all seem to point to code in this patch. If this is indeed the cause, please revert.

I was aware and hopefully this fixes the issue https://github.com/llvm/llvm-project/commit/0361798dbeb6ead0a79ab7985f02da347fce988e

Awesome, thanks. Certainly fixes the compile time failures in my local build. There is still a link-time failure (undefined reference) with my shared libraries build:

tools/clang/tools/extra/clang-tidy/tool/CMakeFiles/obj.clangTidyMain.dir/ClangTidyMain.cpp.o: In function `clang::ast_matchers::internal::matcher_isAllowedToContainClauseKind0Matcher::matches(clang::OMPExecutableDirective const&, clang::ast_matchers::internal::ASTMatchFinder*, clang::ast_matchers::internal::BoundNodesTreeBuilder*) const':
ClangTidyMain.cpp:(.text._ZNK5clang12ast_matchers8internal44matcher_isAllowedToContainClauseKind0Matcher7matchesERKNS_22OMPExecutableDirectiveEPNS1_14ASTMatchFinderEPNS1_21BoundNodesTreeBuilderE[_ZNK5clang12ast_matchers8internal44matcher_isAllowedToContainClauseKind0Matcher7matchesERKNS_22OMPExecutableDirectiveEPNS1_14ASTMatchFinderEPNS1_21BoundNodesTreeBuilderE]+0x50): undefined reference to `llvm::omp::isAllowedClauseForDirective(llvm::omp::Directive, llvm::omp::Clause, unsigned int)'
collect2: error: ld returned 1 exit status

But that may be unrelated to this patch.

Awesome, thanks. Certainly fixes the compile time failures in my local build. There is still a link-time failure (undefined reference) with my shared libraries build:

tools/clang/tools/extra/clang-tidy/tool/CMakeFiles/obj.clangTidyMain.dir/ClangTidyMain.cpp.o: In function `clang::ast_matchers::internal::matcher_isAllowedToContainClauseKind0Matcher::matches(clang::OMPExecutableDirective const&, clang::ast_matchers::internal::ASTMatchFinder*, clang::ast_matchers::internal::BoundNodesTreeBuilder*) const':
ClangTidyMain.cpp:(.text._ZNK5clang12ast_matchers8internal44matcher_isAllowedToContainClauseKind0Matcher7matchesERKNS_22OMPExecutableDirectiveEPNS1_14ASTMatchFinderEPNS1_21BoundNodesTreeBuilderE[_ZNK5clang12ast_matchers8internal44matcher_isAllowedToContainClauseKind0Matcher7matchesERKNS_22OMPExecutableDirectiveEPNS1_14ASTMatchFinderEPNS1_21BoundNodesTreeBuilderE]+0x50): undefined reference to `llvm::omp::isAllowedClauseForDirective(llvm::omp::Directive, llvm::omp::Clause, unsigned int)'
collect2: error: ld returned 1 exit status

But that may be unrelated to this patch.

That's unrelated to this patch, something about ASTMatchers and specifically OpenMP @jdoerfert has changed a few things there, may be worth hitting him up to see.