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

[David Greene via llvm-dev]

We also have a merge-based workflow.  Did you just pull down the most
recent upstream and do a direct merge to your branch, or was there
another step in there to make conflicts less likely, such as renaming
your downstream branch first before merging from upstream?

                       -David

Edd Dawson via llvm-dev <llvm-dev at lists.llvm.org> writes:

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