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

David Greene via llvm-dev llvm-dev at lists.llvm.org
Fri Jul 12 10:34:05 PDT 2019 

Thank you, that's very helpful!

                     -David

Edd Dawson <edd-llvm at mr-edd.co.uk> writes:

> Hi David,
>
> This is what I did:
>
> 1. Merged to just before the commit that changed the naming convention
> in lld/ELF (where we have all of our private patches).
>
> 2. Ran clang-llvm-rename on our downstream lld/ELF and diff-ed the
> result against the upstream change that did the same. The differences
> were only in the areas that we have modified - good. I then committed
> this.
>
> 3. Finally I merged in the upstream change that changed the naming
> convention, simply accepting ours in the case of conflict.
>
> It's surely possible to do it without the intermediate commit, but I
> don't think that it would have bought us much. One side effect is that
> *I* appear in git blame everywhere now. You may want to avoid that, I
> guess.
>
> Thanks,
> Edd
>
> On 2019-07-12 17:05, David Greene wrote:
>> 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

More information about the llvm-dev mailing list