[llvm-dev] RFC: changing variable naming rules in LLVM codebase
Edd Dawson via llvm-dev
llvm-dev at lists.llvm.org
Fri Jul 12 10:05:15 PDT 2019
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
--
Edd Dawson
SN Systems - Sony Interactive Entertainment
http://www.snsystems.com
More information about the llvm-dev
mailing list