[PATCH] D76606: [clang-tidy] Warn on invalid "case" configurations for readability-identifier-naming
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 24 14:00:49 PDT 2020
aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:149-151
+ auto CaseOptionName = (Name + "Case").str();
+ auto CaseValue = Options.get(CaseOptionName, "");
+ auto const caseOptional = fromString(CaseValue);
----------------
Some `auto`s that don't have the type spelled out (also, a top-level `const` that can be dropped).
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:154
+ llvm::errs() << "warning: Invalid case style '" << CaseValue
+ << "' for readability-identifier-naming." << CaseOptionName;
+ constexpr StringRef AcceptibleNames[] = {
----------------
njames93 wrote:
> 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.
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?
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:155
+ << "' for readability-identifier-naming." << CaseOptionName;
+ constexpr StringRef AcceptibleNames[] = {
+#define CASE(NAME, ENUM) NAME,
----------------
acceptible -> acceptable
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:177
+ if (EditDistance < 3)
+ llvm::errs() << ": did you mean '" << Closest << "'\n";
+ else
----------------
njames93 wrote:
> I feel its safer not assuming the fix and instead letting the user modify their configuration, WDYT?
Agreed -- we don't recover in Clang by assuming the fix to the command line option spelling, either.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76606/new/
https://reviews.llvm.org/D76606
More information about the cfe-commits
mailing list