The idea of suppressing naming checks for variables is to support code bases that allow short variables named e.g 'x' and 'i' without prefix/suffixes or casing styles. This was originally proposed as a 'ShortSizeThreshold' however has been made more generic with a regex to suppress identifier naming checks for those that match.
Details
Diff Detail
Event Timeline
Overall pretty good with this, but i feel the name should be ShortNameThreshold, WDYT?
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
176 | Best to use the literal here to make sure we use the unsigned version of Options.get |
Thanks for the review! Agreed with suggestions, submitted updated diff.
Please note I do not have commit access to llvm. Regards.
Oh can you also update the release notes, there is already a largish list of changes to this check there.
Oh to run the test locally, run ninja check-clang-extra-clang-tidy think that's right.
I've updated the release notes and corrected the unit tests to be passing. Thanks again.
Should this be a NamingStyle option instead.
{key: readability-identifier-naming.ParameterShortSizeThreshold, value: 2}
WDYT?
I think that makes a lot of sense -- I can imagine wanting to enforce different identifier lengths depending on whether we're spelling a type name vs a local variable name, etc.
I considered this but thought the configuration and documentation would be quite cumbersome. We could provide a global setting (as already done) and allow refining it further by type if needed? For what it's worth I only need it for local variables, I imagine that would be the main use case.
Or should we simply add this threshold to every type of name, similar to how suffix, prefix and case style have been done?
I think length of names matters differently in different contexts. I think three-letter identifiers as a class name should probably be a rarity, but may be more reasonable as a local variable. I think one-letter local variables are generally a bad thing, except for loop induction variables. That sort of thing. (And we may someday want to add more specific naming categories like "loop induction variable".) So I think adding the threshold to all the different name kinds is a more flexible approach.
I updated it to use XXShortSizeThreshold.
But I was thinking.. perhaps a more generic approach could be "xxIgnoreRegex", this would allow you to do things such as :
VariableIgnoreRegex: ([a-z]{2}|index)
ClassIgnoreRegex: (Special_CASE|CommonOther.*)
etc.
it would be more generic while also allowing for excluding short names.
I don't have a strong opinion on that approach. It would be a more flexible approach and that's appealing, but it also means writing regular expressions which is a bit unappealing because those aren't obvious to everyone and it's unclear what the performance characteristics would be of running those regexes on every identifier in the program. I'm curious if @alexfh or @njames93 (or others) have opinions on this.
For the record, when profiling this (and other RenamerClangTidy based checks) are the slowest(by some margin) checks to run.
This check also uses regex to determine if identifiers are correctly named and to split words in identifiers if they need changing.
Based off that I don't think there is huge performance concerns for using a regex for a couple of styles.
Regexes may be a bit of a slippery slope as any identifier scheme could be represented as a regex (casing, prefix, suffix etc) but it's not practical to provide a fix or suggestion with a regex.
For example with a 'ShortThreshold' a fix could offer to change the casing/xffix or to trim the variable length. With a regex it's less about making a naming rule and more about providing a workaround for problematic edge cases. As such should be used sparingly.
I think the difficulty in writing them is not too much of a deal breaker. Providing an example for the equivalent config options would be just as useful as the config option itself. e.g
"For example, to ignore names with a length less than 3 use: \w{1,3}' "
I assume the ^ and $ would be implied by the matcher. If they want anything more complex they'd be effectively asking for regex support anyway.
Updated diff to use 'IgnoredRegexp'
I see that a regexp has been used elsewhere for some other checks (macro usage, AllowedRegexp).
(as a side note, I see that AllowedRegexp string is getting compiled on every match rather than pre-compiling, the lack of a copy constructor on llvm::Regex makes it trickier to work with)
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
141–143 | Making this change removes the need to NamingStyle to be copy constructable/assignable. | |
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h | ||
61 | Potentially save an allocation. Also is it wise to warn a user on an invalid regex, unfortunately clang-tidy doesnt support diags with no source location but could still output to llvm::errs. |
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
141–143 | Thanks - was looking for a solution to this. |
LGTM, before you push this, can you edit the summary and title as its no longer about just the length of the name. Otherwise the commit message will likely lead to confusion on whats being added.
Thanks for the reviews everyone, I have updated the title and summary.
Unfortunately I don't have push access to llvm - could someone help to push this please?
Just one last small nit.
// You can ignore this, nothing to do with this patch.
All these changes in the documentation tell me we should probably restructure the documentation for this check.
Right now we have NxM lines of documentation for each combination of style kind(N) and style option(M).
This could probably be changed so we could describe each style option at the top of the documentation (<StyleKind>Case, <StyleKind> Prefix, ...)
Then we could describe each StyleKind afterwards.
.. option:: ClassCase, option::ClassPrefix... Transforms class names .. code-block:: c++ class <Ident> { }; .. option:: FunctionCase, option::FunctionPrefix... Transforms class names .. code-block:: c++ void <Ident>(int Param0);
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
149 | Dont need to use str() here |
Yes I thought the same wrt the doco. There is also a hungarian notation config option in the works which does the same thing.
I nearly reworked it but figured it would be best done under a separate patch
It does have the potential to be less searchable through google for specific options. Automatic generation could be used instead if that is a problem.
There's a build failure from this merge, looks like a typo:
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
index b744d21b6ff8..68c0abfcbdf6 100644
- a/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -680,7 +680,7 @@ After:
When defined, the check will ensure enumeration names will add the prefixed with the given value (regardless of casing).
-.. option:: EnumConstantIgnoredRegexp
+.. option:: EnumIgnoredRegexp
Identifier naming checks won't be enforced for enumeration names matching this regular expression.
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst | ||
---|---|---|
683 |
Potentially save an allocation.
Also is it wise to warn a user on an invalid regex, unfortunately clang-tidy doesnt support diags with no source location but could still output to llvm::errs.