Page MenuHomePhabricator

[clang-tidy] Change checks that take enum configurations to use a new access method.
ClosedPublic

Authored by njames93 on Mar 23 2020, 6:15 AM.

Details

Summary

Change all checks that take enums as configuration to use enum specific methods in ClangTidyCheck::OptionsView.

Diff Detail

Event Timeline

njames93 created this revision.Mar 23 2020, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2020, 6:15 AM
njames93 marked 2 inline comments as done.Mar 23 2020, 6:25 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
151

Unsure about hardcoding the check-name as it could be ran under an alias. However there is no way for a ClangTidyCheck to get the name its ran as. ClangTidyCheck::CheckName is private, maybe a protected getter would be wise.

174

I feel its safer not assuming the fix and instead letting the user modify their configuration, WDYT?

njames93 added a project: Restricted Project.Mar 23 2020, 6:25 AM

I'm not too sure how many other checks are like this, but I feel that this functionality could maybe be brought out of this check and into the Options to let more checks that take enum like configurations to use it.

aaron.ballman added inline comments.Mar 24 2020, 1:33 PM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
146–159

Some autos that don't have the type spelled out (also, a top-level const that can be dropped).

151

I agree that we should not hardcode the name of the check. I actually wonder if a better approach here is to call Context->diag() to generate an actual diagnostic?

152

acceptible -> acceptable

174

Agreed -- we don't recover in Clang by assuming the fix to the command line option spelling, either.

njames93 marked 3 inline comments as done.Mar 24 2020, 2:44 PM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
146–159

Copying from what was already there.

151

A few issues with that approach, firstly the ClangTidyCheck doesn't expose the ClangTidyContext to any derived classes. the other issue is it requires a SourceLocation to emit a diagnostic, Passing an invalid SourceLocation effectively suppresses any output, likewise any other SourceLocation will just confuse users.
I have got another implementation in the works that creates new methods in ClangTidyCheck::OptionsView that takes a mapping from StringRef to Enum and uses that for reading (and writing) enums from the configuration. This has access to the CheckName and can distinguish if the enum was read from local or global and adjust accordingly.

njames93 planned changes to this revision.Apr 1 2020, 3:54 PM
njames93 updated this revision to Diff 255027.Apr 4 2020, 4:32 AM
  • Change checks that take enum configurations to use a new access method.
njames93 retitled this revision from [clang-tidy] Warn on invalid "case" configurations for readability-identifier-naming to [clang-tidy] Change checks that take enum configurations to use a new access method..Apr 4 2020, 4:34 AM
njames93 edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Apr 7 2020, 11:23 AM
This revision was automatically updated to reflect the committed changes.