.clang-tidy: Push variable related readability-identifier-naming options down to projects

[MaskRay]

Summary

See https://discourse.llvm.org/t/top-level-clang-tidy-options-and-variablename-suggestion-on-codingstandards/58783 :
the variable naming related top-level readability-identifier-naming options
do not reflect the truth.

{llvm,clang,clang-tools-extra,polly,bolt} use CamelCase but most
other top-level projects use camelBack or disable
readability-identifier-naming (clang). It makes more sense to push down the
related options to llvm, clang-tools-extra, bolt.
Note: clang/ has explicitly disabled readability-identifier-naming.

[dblaikie]

I think it should probably stay as-is, given this is the documented LLVM project naming convention: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly - a bit of extra friction for subprojects to opt-out of that convention seems suitable to me/should serve as documentation that these projects are opting out of the LLVM style in this way.

[dblaikie]

(eg: new projects should benefit from the LLVM default- less chance of further diversification of naming conventions)

[MaskRay]

In D108265#2951222, @dblaikie wrote:

I think it should probably stay as-is, given this is the documented LLVM project naming convention: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly - a bit of extra friction for subprojects to opt-out of that convention seems suitable to me/should serve as documentation that these projects are opting out of the LLVM style in this way.

clang/, flang/, lld/, mlir/, lldb/, compiler-rt have opted out the variable style or disabled the check.

libunwind currently suffers from the variable name style.

I don't find {libcxx,libcxxabi}/.clang-tidy but they clearly don't use Variablename.

(eg: new projects should benefit from the LLVM default- less chance of further diversification of naming conventions)

That's why I kept other options in the top-level:)

[mehdi_amini]

+1 on arguments from @dblaikie : I'm not sure I understand the motivation for this change right now?

[MaskRay]

The number of top-level projects using VariableName is smaller than the number of projects not using the style.
The top-level variable style just provoked projects to either override the options (flang/, lld/, mlir/) or disable the check.
VariableName is not even a suitable suggestion for new projects.

So the VariableName setting does not belong to the top-level. llvm/ and clang-tools-extra/ should set it by themselves.

CodingStandards.rst: "Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)."

This applies to llvm/, clang/, clang-tools-extra/ (and perhaps something I missed), but is not true for many other projects (flang,lld,lldb,mlir,libcxx,libcxxabi,libunwind,compiler-rt,...)

[dblaikie]

In D108265#2952555, @MaskRay wrote:

The number of top-level projects using VariableName is smaller than the number of projects not using the style.
The top-level variable style just provoked projects to either override the options (flang/, lld/, mlir/) or disable the check.
VariableName is not even a suitable suggestion for new projects.

So the VariableName setting does not belong to the top-level. llvm/ and clang-tools-extra/ should set it by themselves.

CodingStandards.rst: "Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)."

This applies to llvm/, clang/, clang-tools-extra/ (and perhaps something I missed), but is not true for many other projects (flang,lld,lldb,mlir,libcxx,libcxxabi,libunwind,compiler-rt,...)

I think it applies to the LLVM umbrella/project as a whole - and should apply to any new projects. (I think it's a mistake that projects started that did not adhere to this naming convention (the same as if they didn't adhere to other aspects of the LLVM coding standard) and created divergence where the coding standards are meant to avoid that/promote consistency)

I think it's appropriate for the top level default to match what's described in the LLVM Coding Standards document & to change that document should be an llvm-dev discussion. (I thought there was one a while back, though I don't recall any specific conclusions coming out of that)

[MaskRay]

In D108265#2953305, @dblaikie wrote:

In D108265#2952555, @MaskRay wrote:

The number of top-level projects using VariableName is smaller than the number of projects not using the style.
The top-level variable style just provoked projects to either override the options (flang/, lld/, mlir/) or disable the check.
VariableName is not even a suitable suggestion for new projects.

So the VariableName setting does not belong to the top-level. llvm/ and clang-tools-extra/ should set it by themselves.

CodingStandards.rst: "Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)."

This applies to llvm/, clang/, clang-tools-extra/ (and perhaps something I missed), but is not true for many other projects (flang,lld,lldb,mlir,libcxx,libcxxabi,libunwind,compiler-rt,...)

I think it applies to the LLVM umbrella/project as a whole - and should apply to any new projects. (I think it's a mistake that projects started that did not adhere to this naming convention (the same as if they didn't adhere to other aspects of the LLVM coding standard) and created divergence where the coding standards are meant to avoid that/promote consistency)

I think it only applies to code which is currently using VariableName.

In https://llvm.org/docs/Proposals/VariableNames.html related discussions, people all agree that VariableName was a mistake.
(Some people took the stance that "renaming variables would cause churn and downstream maintenance burden, so don't do")

But new code doesn't need to use the (inferior) style.

I think it's appropriate for the top level default to match what's described in the LLVM Coding Standards document & to change that document should be an llvm-dev discussion. (I thought there was one a while back, though I don't recall any specific conclusions coming out of that)

As-is, this caused trouble to many projects including flang, mlir, lld, lldb, libunwind. I think libcxx/libcxxabi would be affected as well unless they explicitly override the toplevel options.

[dblaikie]

The current state of the root .clang-tidy accurately reflects the LLVM Coding Conventions as documented, which applies to LLVM and its subprojects (some subprojects have diverged from this standard).

The place for a discussion to change the naming convention is not here. It is on llvm-dev, like the original discussion that spawned the (not-yet-accepted, as it says in its opening sentence) linked proposal around variable naming.

[MaskRay]

Raised https://lists.llvm.org/pipermail/llvm-dev/2021-August/152210.html ("Top-level .clang-tidy options and VariableName suggestion on CodingStandards")

[mehdi_amini]

the original discussion that spawned the (not-yet-accepted, as it says in its opening sentence) linked proposal around variable naming.

+1 with David again: if the proposal gets accepted, then this patch makes sense to me.

[MaskRay]

rebase

[MaskRay]

Created D140585 to update llvm/docs/CodingStandards.rst. Changed the summary to mention https://discourse.llvm.org/t/top-level-clang-tidy-options-and-variablename-suggestion-on-codingstandards/58783

[Amir]

LGTM for BOLT

[MaskRay]

I'll wait a bit and land this.

[dblaikie]

My objections to this initially still seem to stand - the discourse thread doesn't appear to have shown more consensus & I still don't think we should remove a top level naming convention entirely. We should have a convention documented and specified in the top level format file - lest new subprojects pick new/different conventions and increase fragmentation & difficulty for developers moving between LLVM subprojects.

[dblaikie]

(at least if/when D140585 sticks, then the format should be reflected at the top level, not omitted/left to subprojects to specify - then/at the same time, the explicit naming convention specified in mlir, compiler-rt, etc, should be removed to depend on the top level one)

.clang-tidy

 Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-const-correctness,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,readability-identifier-naming'
 CheckOptions:
   - key:             readability-identifier-naming.ClassCase
     value:           CamelCase
   - key:             readability-identifier-naming.EnumCase
     value:           CamelCase
   - key:             readability-identifier-naming.FunctionCase
     value:           camelBack
   # Exclude from scanning as this is an exported symbol used for fuzzing
   # throughout the code base.
   - key:             readability-identifier-naming.FunctionIgnoredRegexp
     value:           "LLVMFuzzerTestOneInput"
-  - key:             readability-identifier-naming.MemberCase
-    value:           CamelCase
-  - key:             readability-identifier-naming.ParameterCase
-    value:           CamelCase
   - key:             readability-identifier-naming.UnionCase
     value:           CamelCase
-  - key:             readability-identifier-naming.VariableCase
-    value:           CamelCase
   - key:             readability-identifier-naming.IgnoreMainLikeFunctions
     value:           1
   - key:             readability-redundant-member-init.IgnoreBaseInCopyConstructors
     value:           1
   - key:             modernize-use-default-member-init.UseAssignment
     value:           1

bolt/.clang-tidy

+Checks: 'readability-identifier-naming'
+InheritParentConfig: true
+CheckOptions:
+  - key:             readability-identifier-naming.ParameterCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.UnionCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.VariableCase
+    value:           CamelCase

clang-tools-extra/.clang-tidy

+Checks: 'readability-identifier-naming'
+InheritParentConfig: true
+CheckOptions:
+  - key:             readability-identifier-naming.ParameterCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.UnionCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.VariableCase
+    value:           CamelCase

llvm/.clang-tidy

+Checks: 'readability-identifier-naming'
 InheritParentConfig: true
+CheckOptions:
+  - key:             readability-identifier-naming.ParameterCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.UnionCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.VariableCase
+    value:           CamelCase

polly/.clang-tidy

+Checks: 'readability-identifier-naming'
+InheritParentConfig: true
+CheckOptions:
+  - key:             readability-identifier-naming.ParameterCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.UnionCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.VariableCase
+    value:           CamelCase