CodingStandards: restrict CamelCase variable names guideline to llvm/clang/clang-tools-extra/polly/bolt

[MaskRay]

Summary

See https://discourse.llvm.org/t/top-level-clang-tidy-options-and-variablename-suggestion-on-codingstandards/58783 ,
the CamelCase variable names guideline does not reflect the truth:
flang, libc, libclc, libcxx, libcxxabi, libunwind, lld, mlir, openmp,
and pstl use camelCase.

[dblaikie]

FWIW, I don't think there's anymore consensus for this now than when this was proposed. The link in the patch description here is a thread where it's mostly you and me discussing it, without any closure - and a few other folks chiming in with their difficulties. And all of this seems to have had little overlap/involvement of the formal proposal, that's still outstanding, in https://reviews.llvm.org/D59251 / https://llvm.org/docs/Proposals/VariableNames.html .

I'm not sure if @mehdi_amini meant their approval to be "this is good to commit in general" or "for their part, it looks OK, but other folks need buy in"?

[mehdi_amini]

In D140585#4019417, @dblaikie wrote:

I'm not sure if @mehdi_amini meant their approval to be "this is good to commit in general" or "for their part, it looks OK, but other folks need buy in"?

That’s how I meant it: sorry I should have left a comment about getting more approvals to reflect better consensus.

I am ok with documenting this right now because it is the actual status quo that matches the reality of the codebase and I am not aware of an active work stream to go in another direction.

[MaskRay]

In D140585#4019417, @dblaikie wrote:

FWIW, I don't think there's anymore consensus for this now than when this was proposed. The link in the patch description here is a thread where it's mostly you and me discussing it, without any closure - and a few other folks chiming in with their difficulties. And all of this seems to have had little overlap/involvement of the formal proposal, that's still outstanding, in https://reviews.llvm.org/D59251 / https://llvm.org/docs/Proposals/VariableNames.html .

I'm not sure if @mehdi_amini meant their approval to be "this is good to commit in general" or "for their part, it looks OK, but other folks need buy in"?

The updated wording matches the current status quo and seems the best resolution for today.

https://reviews.llvm.org/D59251 / https://llvm.org/docs/Proposals/VariableNames.html are proposing changing the status quo which is not what I am proposing. I think the coding standards should just reflect the truth for now.

[dblaikie]

In D140585#4019532, @mehdi_amini wrote:

In D140585#4019417, @dblaikie wrote:

I'm not sure if @mehdi_amini meant their approval to be "this is good to commit in general" or "for their part, it looks OK, but other folks need buy in"?

That’s how I meant it: sorry I should have left a comment about getting more approvals to reflect better consensus.

@MaskRay Could you please revert until this ^ is addressed?

It doesn't look like https://discourse.llvm.org/t/top-level-clang-tidy-options-and-variablename-suggestion-on-codingstandards/58783 found/created more concensus that was (seems to be still is) missing from D108265 that spawned that discourse thread.

[MaskRay]

In D140585#4027224, @dblaikie wrote:

In D140585#4019532, @mehdi_amini wrote:

In D140585#4019417, @dblaikie wrote:

I'm not sure if @mehdi_amini meant their approval to be "this is good to commit in general" or "for their part, it looks OK, but other folks need buy in"?

That’s how I meant it: sorry I should have left a comment about getting more approvals to reflect better consensus.

@MaskRay Could you please revert until this ^ is addressed?

It doesn't look like https://discourse.llvm.org/t/top-level-clang-tidy-options-and-variablename-suggestion-on-codingstandards/58783 found/created more concensus that was (seems to be still is) missing from D108265 that spawned that discourse thread.

I am not sure about reverting this clarification patch. This patch isn't a policy change.

In first few paragraphs there is a statement about naming convention.
"""
Note that some code bases (e.g. `libc++`) have special reasons to deviate
from the coding standards. For example, in the case of `libc++`, this is
because the naming and other conventions are dictated by the C++ standard.

There are some conventions that are not uniformly followed in the code base
(e.g. the naming convention).
"""

Previously, the description of variable names was clearly incorrect for all but llvm/clang/clang-tools-extra.

[aaron.ballman]

In D140585#4027381, @MaskRay wrote:

In D140585#4027224, @dblaikie wrote:

In D140585#4019532, @mehdi_amini wrote:

In D140585#4019417, @dblaikie wrote:

I'm not sure if @mehdi_amini meant their approval to be "this is good to commit in general" or "for their part, it looks OK, but other folks need buy in"?

That’s how I meant it: sorry I should have left a comment about getting more approvals to reflect better consensus.

@MaskRay Could you please revert until this ^ is addressed?

It doesn't look like https://discourse.llvm.org/t/top-level-clang-tidy-options-and-variablename-suggestion-on-codingstandards/58783 found/created more concensus that was (seems to be still is) missing from D108265 that spawned that discourse thread.

I am not sure about reverting this clarification patch. This patch isn't a policy change.

In first few paragraphs there is a statement about naming convention.
"""
Note that some code bases (e.g. `libc++`) have special reasons to deviate
from the coding standards. For example, in the case of `libc++`, this is
because the naming and other conventions are dictated by the C++ standard.

There are some conventions that are not uniformly followed in the code base
(e.g. the naming convention).
"""

Previously, the description of variable names was clearly incorrect for all but llvm/clang/clang-tools-extra.

I think there is a somewhat deeper topic here. My initial understanding of the coding standards and related policies was that they're the standard for all LLVM projects. However, as you point out, they're really not, but those differences go beyond just coding style and into things like how build bots work, how code reviews work, documentation expectations, revert expectations, etc. LLVM, Clang and clang-tools-extra all generally follow the LLVM project guidelines while other projects such as libc++, lld, lldb, act more as separate-but-related projects with their own (sometimes undocumented) approaches. For example, our revert policy implies that it's reasonable for e.g., libc++ to revert a correct, conforming diagnostic change in Clang because it breaks a libc++ test which was testing the specific wording of that diagnostic. However, in practice, our preference is usually for the libc++ folks to fix the test rather than revert a correct, conforming change to Clang, but we don't really make this clear in our policies.

Personally, I think we want to give our subcommunities the autonomy needed to do what meets their needs, and that includes diverging from the primary documented policies. But then again, my mental model has changed somewhat about how I view projects in the monorepo. To me, Clang, LLVM, and clang-tools-extra are "upstream" tools while libc++, lldb, lld, etc are "downstreams" that consume the upstream tools. So to me, it would make sense for the developer policies apply to Clang, LLVM, and clang-tools-extra, while other projects are expected to document their deviations from the main policies. Because of that, I don't think we should do this by adding exceptions to the main policy (as done here) because that makes the main policies harder to understand. Instead, I'd rather we linked from the main policy back to various project policies (and vice versa) so it's more clear that there's a distinction between projects in the monorepo. This also makes it less contentious for these "downstream" projects to deviate from the main policies -- you don't have to convince the whole community the idea is good once the subcommunity has established a preference. Alternatively, we should get everyone onto the same policies.

[dblaikie]

Regardless of the content of the patch, I think it should be reverted as it wasn't fully approved - more consensus is suitable for a patch like this & it wasn't @mehdi_amini's intent to sufficient approval alone.

In D140585#4027381, @MaskRay wrote:

In D140585#4027224, @dblaikie wrote:

In D140585#4019532, @mehdi_amini wrote:

In D140585#4019417, @dblaikie wrote:

I'm not sure if @mehdi_amini meant their approval to be "this is good to commit in general" or "for their part, it looks OK, but other folks need buy in"?

That’s how I meant it: sorry I should have left a comment about getting more approvals to reflect better consensus.

@MaskRay Could you please revert until this ^ is addressed?

It doesn't look like https://discourse.llvm.org/t/top-level-clang-tidy-options-and-variablename-suggestion-on-codingstandards/58783 found/created more concensus that was (seems to be still is) missing from D108265 that spawned that discourse thread.

I am not sure about reverting this clarification patch. This patch isn't a policy change.

It sets the policy for new directories to be the newer one, not the LLVM/Clang one, which is a policy change so far as I can see.

If it were stated in the inverse - that those particular directories use the other convention, and "other directories" (including llvm/clang and any new directories) use the upper-start, that to me would be documenting existing practice, not a policy change.

In D140585#4028898, @aaron.ballman wrote:

I think there is a somewhat deeper topic here. My initial understanding of the coding standards and related policies was that they're the standard for all LLVM projects. However, as you point out, they're really not, but those differences go beyond just coding style and into things like how build bots work, how code reviews work, documentation expectations, revert expectations, etc. LLVM, Clang and clang-tools-extra all generally follow the LLVM project guidelines while other projects such as libc++, lld, lldb, act more as separate-but-related projects with their own (sometimes undocumented) approaches.

I think of these as bugs, rather than features - maybe we document them as existing bugs, or allow them as exceptions, but I think we should be striving for more consistency. The ability to move code between subprojects (eg: promoting some data structure to llvm's ADT library) would benefit from being smoother, and developers ability to move easily between subprojects without relearning conventions would be valuable.

Maybe we go ultimately end up with this patch/direction - that new projects use the new convention, and Clang/LLVM are left alone for now, maybe to be cleaned up/migrated later. I personally don't think the difference in naming is sufficiently valuable to justify this inconsistency (& I think it was a mistake to allow new subprojects with these deviating naming conventions - those should've been fixed before integrating them), and probably by LOC LLVM+Clang still dominate the project, so by LOC maybe we have more code using the old convention than the new, so a new subproject would have greater consistency across the project by using the old one (not to mention that, as you said, @aaron.ballman - the leaves are different from the core - most subprojects interact with LLVM, but not with each other - so for any given subproject, the code you interact with is probably dominated by LLVM and its naming convention).

[mehdi_amini]

I personally don't think the difference in naming is sufficiently valuable to justify this inconsistency (& I think it was a mistake to allow new subprojects with these deviating naming conventions - those should've been fixed before integrating them)

FYI, speaking for MLIR, the only reason it ended up with a different naming convention is because we thought LLVM was about to adopt the proposed new one at the time and we thought we'd avoid a later renaming for this codebase. The intent was to be aligned with LLVM and the current result is more unfortunate than intended.

[probinson]

So, the real problem is, once somebody managed to herd the cats all into the same direction (or close enough), the consensus was "no mass conversion to the new style." Which naturally left everything inconsistent.

I wasn't two years out of college before I was confronted by a page of code with something like 6 different styles on the same 24-line screen. Ever since (40 years!), I have understood that the critical thing is that _everybody uses the same style_, and it's much less important what that style actually is. Trying to go in a direction of "there are different styles and we live with it" really doesn't help anyone.

Further discussion really belongs on Discourse, not in a code review.

[dblaikie]

In D140585#4029712, @mehdi_amini wrote:

I personally don't think the difference in naming is sufficiently valuable to justify this inconsistency (& I think it was a mistake to allow new subprojects with these deviating naming conventions - those should've been fixed before integrating them)

FYI, speaking for MLIR, the only reason it ended up with a different naming convention is because we thought LLVM was about to adopt the proposed new one at the time and we thought we'd avoid a later renaming for this codebase. The intent was to be aligned with LLVM and the current result is more unfortunate than intended.

Ah, that's helpful to understand - thanks!

llvm/docs/CodingStandards.rst

 
 In general, names should be in camel case (e.g. ``TextFileReader`` and
 ``isLValue()``).  Different kinds of declarations have different rules:
 
 * **Type names** (including classes, structs, enums, typedefs, etc) should be
   nouns and start with an upper-case letter (e.g. ``TextFileReader``).
 
 * **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``).
+  be camel case.  In ``llvm/``, ``clang/``, ``clang-tools-extra/``, ``polly/``,
+  and ``bolt/``, the name should start with an upper-case letter (e.g.
+  ``Leader`` or ``Boats``); start with a lower case-letter letter for other
+  directories.
 
 * **Function names** should be verb phrases (as they represent actions), and
   command-like function should be imperative.  The name should be camel case,
   and start with a lower case letter (e.g. ``openFile()`` or ``isFoo()``).
 
 * **Enum declarations** (e.g. ``enum Foo {...}``) are types, so they should
   follow the naming conventions for types.  A common use for enums is as a
   discriminator for a union, or an indicator of a subclass.  When an enum is