llvm dev - Aug 2021 - Top-level .clang-tidy options and VariableName suggestion on CodingStandards

On Thu, Aug 19, 2021 at 11:23 AM Fāng-ruì Sòng <maskray at google.com>
wrote:
> On Thu, Aug 19, 2021 at 10:55 AM David Blaikie <dblaikie at
gmail.com> wrote:
> >
> > On Wed, Aug 18, 2021 at 4:50 PM Fāng-ruì Sòng via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >>
> >> Hi all,
> >>
> >> I created a .clang-tidy patch https://reviews.llvm.org/D108265 to
push
> >> the variable naming related options from
> >> top-level .clang-tidy down to the projects actually using the
style
> >> (llvm/, clang-tools-extra/).
> >>
> >> The rationale is: uppercase VariableName doesn't really
reflect the
> >> reality for many projects and (in my view) is a bad
> >> suggestion for new projects. My summary about the reality:
> >>
> >> * Use VariableName and have readability-identifier-naming check:
llvm,
> >> clang-tools-extra, parallel-libs, polly
> >> * Use VariableName but disable readability-identifier-naming:
clang
> >> * Use lowercase variable names and disable
> >> readability-identifier-naming: (mostly lowercase) compiler-rt,
flang,
> >> lldb
> >> * Use lowercase variable names and enable
> >> readability-identifier-naming: libc, libclc, libcxx, libcxxabi,
> >> libunwind, lld, mlir, openmp, pstl
> >>
> >> (I noticed the issue because some Phabricator bot wanted me to use
> >> VariableName for a libunwind patch of mine.
> >> The request looked unreasonable to me. I could add
> >> libunwind/.clang-tidy, but I figured out the top-level .clang-tidy
> >> problem.)
> >>
> >> From my vague memory about previous discussion on variable names,
most
> >> people agree that VariableName was a mistake and new projects do
not
> >> necessarily need to follow.
> >
> >
> > Might be worth going back over those discussions and
summarizing/linking
> to supporting archives, etc, so we don't rehash the whole conversation
> again here? (& cc'ing interested parties so they don't have to
stumble
> across this thread or miss it by accident)
>
> https://llvm.org/docs/Proposals/VariableNames.html#introduction has a
> summary of the 2019 discussion.
>
> "There is some agreement that the current rule is broken
> [LattnerAgree] [ArsenaultAgree] [RobinsonAgree] and that acronyms are
> an obstacle to reading new code [MalyutinDistinguish] [CarruthAcronym]
> [PicusAcronym]. There are some opposing views [ParzyszekAcronym2]
> [RicciAcronyms]."
>
> Perhaps "some" can be converted to "majority" or
"full".
> Everyone thinks VariableName was a mistake.
>
"everyone" might be too strong a statement - there's no real way
to
determine that, effectively.

> (The "opposing views" were about whether it was worthwhile to
switch
> to a variant of lowerCase/lower_case for existing code (mostly llvm/,
> clang/, clang-tools-extra/).)
>
Personally, I don't find one naming convention over another (of the ones
being considered) to be sufficiently different in usability to justify
making any changes. But partly to justify making lots of changes to LLVM
core, or, in the absence of such changes, to create more divergence between
that code and other subprojects (because most folks will work on LLVM
core + some subprojects, so consistency with LLVM core is pretty important).

> >> However, one feedback is that
> >>
>
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> >> says
> >>
> >> > Variable names should be nouns (as they represent state). The
name
> should be camel case, and start with an uppercase letter (e.g. Leader or
> Boats).
> >>
> >> so the top-level .clang-tidy should keep the `VariableName`
suggestion
> >> and should apply to new top-level projects.
> >>
> >> Request
> >> ======> >>
> >> I would like to get feedback on the following two points:
> >>
> >> 1. Should we proceed with the .clang-tidy change
> >> https://reviews.llvm.org/D108265 ?
> >> 2. Should the VariableName wording on CodingStandards.html be
> >> clarified/relaxed? llvm, clang, and clang-tools-extra can keep
using
> >> it but lowercase names should also be allowed.
> >
> >
> > Further thought: I don't think (1) should proceed as-is no matter
the
> decision on (2). We should have an LLVM umbrella naming convention (I
don't
> think the number of projects that use one or the other should be a vote in
> favor or against - LLVM Core is still the core of the umbrella project and
> has more weight here than other subprojects (not the only thing that
> matters, but part of it), removing any naming convention I think would be
> unhelpful.
> > & I don't think (2) should be "use either of these naming
conventions"
> it should be "this is the naming convention" perhaps with a note
that LLVM
> Core (& some other places, such as clang-tools-extra) uses a different
one
> for historical reasons, for instance.
>
>
> There were debates about whether camelCase/snake_case should be used
> for variable names,
> Personally I am fine with either one but slightly prefer snake_case.
> New projects using either form will be fine to me.
>
> For (1), I have some doubt whether there will be an LLVM umbrella
> naming convention for variable names,
> but I **don't** consider the lack of convergence (to either camelCase
> or snake_case) an argument for keeping VariableName in the in the
> top-level .clang-tidy .
> Actually I strongly oppose to keeping VariableName in the top-level
> .clang-tidy, or new projects adopting VariableName.
>
It's pretty important to me that we don't make it easier to create more
divergence in naming conventions - creating another project that diverges
from LLVM's naming conventions/creates more naming inconsistency seems like
a loss to me. So until there's some buy-in for a desired future direction,
I think new projects should adhere to the existing convention.

I don't think that future direction must include a way to cleanup LLVM - as
Chris Lattner said, maybe it is something like "Even if the community does
not have the appetite to do a huge scale renaming of all the things in
LLVM, it would be interesting to carve out an exception for new code being
written, refactored, or potentially for use in new subprojects." - though I
think that's a choice we should all make together (doesn't require it to
be
unanimous) and carefully. That does produce the inconsistent-with-LLVM-core
issue I'm concerned about, but maybe with a path forward to consistency.
Might be worth talking about whether people would be comfortable with more
active cleanup - not necessarily one huge renaming, but be willing to
accept patches that rename things without necessarily being pieces of code
being actively refactored for other reasons. That'd help alleviate the
"now
we're going to have N different conventions based on when a given
subproject was introduced", which increases friction when moving between
subprojects (as zturner pointed out in some of the discussion/code review
of style changes proposed previously). - Dave
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210819/c4552bb9/attachment.html>

Hi,

Just a couple cents from a relative newcomer to LLVM, please take them
with their appropriate worth ;)

On Thu 19 Aug 2021 21:21, David Blaikie via llvm-dev <llvm-dev at
lists.llvm.org> writes:
> It's pretty important to me that we don't make it easier to create
> more divergence in naming conventions - creating another project that
> diverges from LLVM's naming conventions/creates more naming
> inconsistency seems like a loss to me. So until there's some buy-in
> for a desired future direction, I think new projects should adhere to
> the existing convention.
FWIW I found when switching between lld and llvm to be jarring -- I had
to know that there were two coding conventions, and that one was
documented and one was not.  It wasted a bit of reviewer time pointing
out these issues (though of course the tools helped).
> I don't think that future direction must include a way to cleanup LLVM
> - as Chris Lattner said, maybe it is something like "Even if the
> community does not have the appetite to do a huge scale renaming of
> all the things in LLVM, it would be interesting to carve out an
> exception for new code being written, refactored, or potentially for
> use in new subprojects." - though I think that's a choice we
should
> all make together (doesn't require it to be unanimous) and
> carefully. That does produce the inconsistent-with-LLVM-core issue I'm
> concerned about, but maybe with a path forward to consistency. Might
> be worth talking about whether people would be comfortable with more
> active cleanup - not necessarily one huge renaming, but be willing to
> accept patches that rename things without necessarily being pieces of
> code being actively refactored for other reasons. That'd help
> alleviate the "now we're going to have N different conventions
based
> on when a given subproject was introduced", which increases friction
> when moving between subprojects (as zturner pointed out in some of the
> discussion/code review of style changes proposed previously). - Dave
Here again I found problems when switching to clang -- you end up with
different naming conventions per file (what is the convention for
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/ConstantEmitter.h
?  When making a patch, which convention to use?) and sometimes even
within a file (consider
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.h#L4819-L4831
vs
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.h#L1484-L1491).
So when making a patch I have to know not only the coding convention but
also the direction that things are supposed to be going, and there does
not appear to be project-wide consensus on that.

Also just FWIW, when I was working on Firefox they changed coding
conventions but did so with a big bang switch.  I found it much easier
to navigate -- the situation was at all times clear to me.
>From my limited perspective It Would Be Good™ if llvm would end up in a
situation where the coding conventions were always enforced -- the
situation in clang is perhaps the least desirable one.  And bonus if it
is the same convention across the project :)

Regards,

Andy

First of all, thank you for pushing on this topic. I would agree that
UpperCamelCase naming style was a mistake.

[snip]
> > Further thought: I don't think (1) should proceed as-is no matter
the
> decision on (2). We should have an LLVM umbrella naming convention (I
don't
> think the number of projects that use one or the other should be a vote in
> favor or against - LLVM Core is still the core of the umbrella project and
> has more weight here than other subprojects (not the only thing that
> matters, but part of it), removing any naming convention I think would be
> unhelpful.
>
>> > & I don't think (2) should be "use either of these
naming conventions"
>> it should be "this is the naming convention" perhaps with a
note that LLVM
>> Core (& some other places, such as clang-tools-extra) uses a
different one
>> for historical reasons, for instance.
>>
>
That sounds fair.


> There were debates about whether camelCase/snake_case should be used
>> for variable names,
>> Personally I am fine with either one but slightly prefer snake_case.
>> New projects using either form will be fine to me.
>>
>> For (1), I have some doubt whether there will be an LLVM umbrella
>> naming convention for variable names,
>> but I **don't** consider the lack of convergence (to either
camelCase
>> or snake_case) an argument for keeping VariableName in the in the
>> top-level .clang-tidy .
>> Actually I strongly oppose to keeping VariableName in the top-level
>> .clang-tidy, or new projects adopting VariableName.
>>
>
> It's pretty important to me that we don't make it easier to create
more
> divergence in naming conventions - creating another project that diverges
> from LLVM's naming conventions/creates more naming inconsistency seems
like
> a loss to me. So until there's some buy-in for a desired future
direction,
> I think new projects should adhere to the existing convention.
>
> I don't think that future direction must include a way to cleanup LLVM
-
> as Chris Lattner said, maybe it is something like "Even if the
community
> does not have the appetite to do a huge scale renaming of all the things in
> LLVM, it would be interesting to carve out an exception for new code being
> written, refactored, or potentially for use in new subprojects." -
though I
> think that's a choice we should all make together (doesn't require
it to be
> unanimous) and carefully. That does produce the inconsistent-with-LLVM-core
> issue I'm concerned about, but maybe with a path forward to
consistency.
>
Might be worth talking about whether people would be comfortable with
more
> active cleanup - not necessarily one huge renaming, but be willing to
> accept patches that rename things without necessarily being pieces of code
> being actively refactored for other reasons. That'd help alleviate the
"now
> we're going to have N different conventions based on when a given
> subproject was introduced", which increases friction when moving
between
> subprojects (as zturner pointed out in some of the discussion/code review
> of style changes proposed previously).
>
My view is that changes for purely stylistic reasons hurt *much* more than
switching between projects with different variable naming styles.

LLVM de facto very much has a distributed development model. Even teams
that work close to upstream often have their own branches where changes are
developed before they are ready for Phabricator. Style changes have a
tendency to introduce totally pointless merge failures. Anybody who is
proposing purely stylistic changes is *usually* proposing creating a lot of
busywork for a lot of people. It*s just not worth it.

(Sometimes, stylistic cleanups are truly confined to a single file. In that
case, they may be fine, especially when the file doesn't change very much
in the first place. As soon as you're changing names in header files,
though, you're very likely going to cause problems.)

*Mandating* a move away from UpperCamelCase for *new* code, and *allowing*
(or perhaps *suggesting*) renames away from UpperCamelCase for code that is
being refactored anyway, is the way to go in my opinion.

Cheers,
Nicolai


> - Dave
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210821/e69dcab0/attachment.html>