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

Thu Jul 11 22:05:20 PDT 2019

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 <https://reviews.llvm.org/D64123> 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]
>>>>>>>> https://public-inbox.org/git/20190515214503.77162-8-brho@google.com/T/
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> *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
>>>>>>>> <https://github.com/tensorflow/mlir> 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
>>>>>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Transforms/LoopUnrollAndJam.cpp>
>>>>>>>> , here is another
>>>>>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Parser/Parser.cpp>,
>>>>>>>> here is an example header
>>>>>>>> <https://github.com/tensorflow/mlir/blob/master/include/mlir/IR/Location.h>
>>>>>>>> .
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>>>
>>>>>>>> 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
>>>>>>>> _______________________________________________
>>>>>>>> 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
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190712/a9344335/attachment.html>