Page MenuHomePhabricator

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

Authored by MaskRay on Aug 17 2021, 7:15 PM.

Details

Summary

The variable related top-level readability-identifier-naming options
don't apply to many projects.

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

Diff Detail

Event Timeline

MaskRay requested review of this revision.Aug 17 2021, 7:15 PM
MaskRay created this revision.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 17 2021, 7:15 PM

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.

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

MaskRay added a comment.EditedAug 17 2021, 7:49 PM

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:)

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

MaskRay added a comment.EditedAug 18 2021, 9:48 AM

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,...)

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 added a comment.EditedAug 18 2021, 2:55 PM

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.

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.

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

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.