[llvm-dev] RFC: changing variable naming rules in LLVM codebase

Mon Jul 15 22:54:58 PDT 2019

Hi Edd,

On Fri, Jul 12, 2019 at 10:04 PM Edd Dawson <edd-llvm at mr-edd.co.uk> wrote:

> Hi Rui, all,
>
> Yesterday I brought the variable-renaming commits in to Sony's
> downstream LLD. We have a merge-based flow rather than continually
> rebasing our patch set, but it went reasonably smoothly nevertheless.
>

I'm very glad to hear that!

> The one snag I hit is that the tool initially missed variables mentioned
> in assert()s. I didn't put much time in to investigating this, but I
> presume it's because my compile_commands.json was build with assert()s
> disabled and so the names mentioned in the predicates were invisible to
> clang-llvm-rename. The result was that I ended up with something that
> built cleanly with NDEBUG, but not otherwise. I guess this is
> essentially the same as the #ifdef'd-out code issue you mentioned, but
> its effect is probably more widespread.
>

I experienced the exact same issue when I was creating the renaming change.
The solution was to build lld with `-DCMAKE_BUILD_TYPE=Debug` (or
`-DLLVM_ENABLE_ASSERTIONS=On`), which enables assertions so that clang
would see code inside ASSERT.

> It was easily remedied by building in another configuration and
> reapplying to tool, but it's something others might want to watch out
> for.
>
> Thanks,
> Edd
>
> On 2019-07-12 06:05, Rui Ueyama via llvm-dev wrote:
> > So, I submitted a few patches to rename all variables in lld. If you
> > are interested in how it looks like, pick up any .cpp or .h file from
> > https://github.com/llvm/llvm-project/tree/master/lld.
> >
> > I learned a few things by doing this which will help me do the same
> > thing to other LLVM (sub-)projects.
> >
> >  - Overall a batch tool to rename variables worked reasonably well, so
> > the coding style change is doable.
> >
> >  - There were a few classes that have a member variable Foo and a
> > function foo(), which would conflict after renaming. I rename
> > variables manually before renaming them. That's not a scalable
> > solution, though. For a larger codebase, I'd probably need to automate
> > it by, for example, renaming foo() to getFoo() if there's Foo variable
> > already.
> >
> >  - There were a few variables that would become a reserved word such
> > as `new` or `private` after renaming. I renamed them manually before
> > mass-renaming. For scalability, it probably have to be automated by
> > appending `_` at the end, for example.
> >
> >  - My clang-based tool didn't work for #ifdef'ed-out code, which
> > caused unexpected failures on buildbots after submitting. I don't know
> > how to fix the tool so that the tool can handle code containing
> > #ifdefs, but at least we need to keep it in mind so that we can check
> > it manually.
> >
> >  - LLVM's `/*foo=*/`-style comment to annotate function arguments need
> > to be handled automatically to make the tool scalable. So is the
> > doxygen @parameter.
> >
> >  - Since a variable rename change virtually touches every line of
> > codebase, that would cause massive merge conflicts to downstream repos
> > if we don't do anything to support them. We need to provide a tool and
> > guidance as to how to apply the tool to a out-of-tree repo so that a
> > renaming change is merged smoothly.
> >
> > On Wed, Jul 10, 2019 at 8:24 PM Rui Ueyama <ruiu at google.com> wrote:
> >
> >> Good point, too. I believe I can find lines starting with
> >> `@parameter` and apply the same name conversion rules to identifiers
> >> after `@parameter`. Since lld doesn't use doxygen comments, it is
> >> fine for now, but before moving forward, I'll address that. Thank
> >> you for pointing that out.
> >>
> >> On Wed, Jul 10, 2019 at 8:20 PM Alex Brachet-Mialot
> >> <alexbrachetmialot at gmail.com> wrote:
> >>
> >> Also, now that I think about it, I believe doxygen will fail to
> >> build if the @parameter comments aren't changed to match the new
> >> names, my guess is it case sensitive. So perhaps we will need to
> >> find a way to manually change these names for doxygen comments?
> >>
> >> On Wed, Jul 10, 2019 at 7:12 AM Alex Brachet-Mialot
> >> <alexbrachetmialot at gmail.com> wrote:
> >>
> >> Rui,
> >>
> >> I have created D64474 to change comments explicitly stating the
> >> parameter names for constants ( /*parameterName=*/<constant> ). I
> >> did this by hand to match the new variable names. Do you know if
> >> there would be a way to update these comments with a tool similar to
> >> what you used to change these names? Perhaps it would be much more
> >> difficult, I'm guessing clang's AST doesn't have a way to describe
> >> comments? It's obviously not a huge deal to have these changed it
> >> could be done over time.
> >>
> >> Best,
> >> Alex
> >>
> >> On Wed, Jul 10, 2019 at 12:18 AM Rui Ueyama via llvm-dev
> >> <llvm-dev at lists.llvm.org> wrote:
> >>
> >> Hi Joan,
> >>
> >> On Tue, Jul 9, 2019 at 9:46 PM Joan Lluch <joan.lluch at icloud.com>
> >> wrote:
> >>
> >> Hi Rui,
> >>
> >> I’m totally positive on switching to camelCase convention. In fact
> >> I have been always uncomfortable with the current naming approach.
> >>
> >> My only suggestion/concern is that we should provide a transition
> >> path not only for the trunk code in the repository, but for eventual
> >> out-of-trunk code with implementations of custom architectures. I
> >> have currently a custom backend implemented on top of LLVM 9 and
> >> therefore this change will surely break my code. I assume that
> >> developers affected by this will be able to run the converting tools
> >> to fix their own code.
> >>
> >> The tool that I wrote for lld's style conversion should work for
> >> out-of-trunk code, so as I described in the previous email,
> >> third-party code maintainer should be able to use the tool to
> >> convert the style first in their repositories and then rebase in
> >> order to avoid large merge conflicts.
> >>
> >> The tool needs to be polished to convert other subprojects such as
> >> clang, but I'll keep your request in mind. I'll try my best to
> >> provide a smooth transition path for out-of-trunk code for any
> >> change that I'll submit for the style change.
> >>
> >> John
> >>
> >> On 9 Jul 2019, at 09:23, Rui Ueyama via llvm-dev
> >> <llvm-dev at lists.llvm.org> wrote:
> >>
> >> Thanks, Chris.
> >>
> >> Looks like everybody is at least mildly comfortable with my variable
> >> name renaming change, so I'll to submit that change to lld
> >> subdirectory soon. If you have any objections, please let me know.
> >> Note that this is not the end of my effort but actually the
> >> beginning of it. As Chris said, I believe we should do this to the
> >> entire LLVM codebase. I'm planning to do that directory-by-directory
> >> basis.
> >>
> >> On Tue, Jul 9, 2019 at 2:03 PM Chris Lattner <clattner at nondot.org>
> >> wrote:
> >>
> >> This looks really great to me Rui, and I’m also pleased to see the
> >> positive comments on the review thread.  Thank you for driving this
> >> forward!
> >>
> >> -Chris
> >>
> >> On Jul 4, 2019, at 9:50 PM, Rui Ueyama <ruiu at google.com> wrote:
> >>
> >> Hi all,
> >>
> >> I wrote a tool [1] to batch-rename variable names so that they are
> >> in camelCase, and I applied the tool to lld subdirectory. You can
> >> see my change at https://reviews.llvm.org/D64121. If you have any
> >> comments, please reply.
> >>
> >> If people are happy about this change, I can do the same thing for
> >> other directories including LLVM itself and Clang.
> >>
> >> On Mon, Jun 10, 2019 at 6:34 PM Rui Ueyama <ruiu at google.com> wrote:
> >>
> >> On Mon, Jun 10, 2019 at 6:27 PM Michael Platings
> >> <Michael.Platings at arm.com> wrote:
> >>
> >> Hi Rui,
> >>
> >> As per the provisional plan [1] we’re still at step 1: improving
> >> git blame. The status of this is that there are some fairly mature
> >> patches in the Git project’s queue [2], and I’m hopeful that it
> >> will be accepted in something close to its current form.
> >>
> >> But if you can get started on steps 2 & 3 i.e. making forks
> >> available with the possible changes applied then that would be
> >> great. My hope is that once everyone can see what the options really
> >> look like then it will be easier to reach consensus.
> >>
> >> Sure, I'll try to do that. I'll probably start with finding
> >> identifiers and typenames that will conflict after the naming scheme
> >> change and rename them so that they won't conflict. The number of
> >> such symbols would hopefully be small, and submitting such renaming
> >> changes wouldn't be distracting. After that, I think I can create a
> >> mechanical change to rename variables to see how it will look like.
> >>
> >> Thanks,
> >>
> >> -Michael
> >>
> >> [1]
> >> https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan
> >> [2]
> >>
> >> [2]
> >>
> > https://public-inbox.org/git/20190515214503.77162-8-brho@google.com/T/
> >> [3]
> >>
> >> FROM: Rui Ueyama <ruiu at google.com>
> >> SENT: 07 June 2019 08:42
> >> TO: Chris Lattner <clattner at nondot.org>
> >> CC: Michael Platings <Michael.Platings at arm.com>;
> >> llvm-dev at lists.llvm.org; nd <nd at arm.com>
> >> SUBJECT: Re: [llvm-dev] RFC: changing variable naming rules in LLVM
> >> codebase
> >>
> >> This thread is not active for a while, but I'm still interested in
> >> seeing a change.
> >>
> >> Reading through this thread, looks like we can agree that we want to
> >> change the local variable naming scheme so that they start with a
> >> lowercase letter. Besides that, there were discussions about
> >> lower_case, camelCase, m_ prefix, and each argument seems as
> >> convincing as others. I'm not sure what is the decision making
> >> process in a situation like this.
> >>
> >> I'd personally vote for changing local variables to start with a
> >> lowercase letter and keep other naming conventions as-is to make it
> >> a minimum change.
> >>
> >> As I stated before, I'm happy to make a change to lld to see how a
> >> naming convention change will look like, but in order to do that I
> >> need to get at least a rough consensus to do that. What is a way to
> >> proceed?
> >>
> >> On Sat, May 25, 2019 at 3:00 PM Chris Lattner via llvm-dev
> >> <llvm-dev at lists.llvm.org> wrote:
> >>
> >> Hi Michael,
> >>
> >> I’m still very interested in seeing a change here.  If someone is
> >> interested in seeing a codebase using the proposed camelBack
> >> convention for variables names, the MLIR codebase is public [4] and
> >> uses it.
> >>
> >> If you’re curious to see what this looks like in practice, there
> >> are lots of examples in the codebase, here is an example .cpp file
> >> [5], here is another [6], here is an example header [7].
> >>
> >> We are still working our way through open sourcing logistics (not
> >> all the code is out yet), but we would still like to contribute at
> >> least parts of this to LLVM if the project is interested.  [[This is
> >> just an FYI, not itself a proposal yet - one will be coming when
> >> we’re ready.]]
> >>
> >> -Chris
> >>
> >> On May 21, 2019, at 3:01 AM, Michael Platings via llvm-dev
> >> <llvm-dev at lists.llvm.org> wrote:
> >>
> >> Hi folks,
> >>
> >> Git is on its way to learning how to ignore commits, allowing us to
> >> do variable renaming and other small refactorings without breaking
> >> git blame.
> >>
> >> It's like git-hyper-blame [1] but significantly more powerful as it
> >> uses fuzzy matching to match lines, including lines that may have
> >> been split or joined.
> >>
> >> A preview release of Git with this new feature is at:
> >> https://github.com/mplatings/git/releases/tag/ignore-rev [8]
> >>
> >> Some of you have told me that you already have to spend time running
> >> git blame multiple times to look past uninteresting commits so I'd
> >> love for you to give this feature a try and see if it helps you.
> >> Your feedback will be very valuable.
> >>
> >> Thanks,
> >> -Michael
> >>
> >> [1]
> >>
> >
> https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html
> >> [9]
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >>
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >  _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
> >
> > Links:
> > ------
> > [1] https://reviews.llvm.org/D64123
> > [2] https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan
> > [3]
> > https://public-inbox.org/git/20190515214503.77162-8-brho@google.com/T/
> > [4] https://github.com/tensorflow/mlir
> > [5]
> >
> https://github.com/tensorflow/mlir/blob/master/lib/Transforms/LoopUnrollAndJam.cpp
> > [6]
> > https://github.com/tensorflow/mlir/blob/master/lib/Parser/Parser.cpp
> > [7]
> >
> https://github.com/tensorflow/mlir/blob/master/include/mlir/IR/Location.h
> > [8] https://github.com/mplatings/git/releases/tag/ignore-rev
> > [9]
> >
> https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> --
> Edd Dawson
> SN Systems - Sony Interactive Entertainment
> http://www.snsystems.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190716/557911ba/attachment.html>