llvm dev - Jul 2019 - RFC: changing variable naming rules in LLVM codebase

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

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

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

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

Hello Rui, all,

We recently went through the LLD renaming process at Intel and it went
reasonably well too.
However I encountered one case when the tool seems not to work:

int foo()
{
  int A = 1;
  int B = 2;
#ifdef __WIN32
  int C = A;
#else
  int C = B;
#endif
  return C;
}

After running the tool only one of #ifdef’ed branch will be renamed. Another
will be left broken.
Is there a plan how we will deal with this?

And one more question:
What is the plan of renaming variables in llvm and clang? Will it be done
gradually, i.e. directory by directory?

Best regards,
Denis Bakhvalov.

From: llvm-dev <llvm-dev-bounces at lists.llvm.org<mailto:llvm-dev-bounces
at lists.llvm.org>> On Behalf Of Rui Ueyama via llvm-dev
Sent: Monday, July 15, 2019 22:55
To: Edd Dawson <edd-llvm at mr-edd.co.uk<mailto:edd-llvm at
mr-edd.co.uk>>
Cc: llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at
lists.llvm.org>>; nd <nd at arm.com<mailto:nd at arm.com>>
Subject: Re: [llvm-dev] RFC: changing variable naming rules in LLVM codebase

Hi Edd,

On Fri, Jul 12, 2019 at 10:04 PM Edd Dawson <edd-llvm at
mr-edd.co.uk<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto:ruiu at google.com>> wrote:
>>
>> On Mon, Jun 10, 2019 at 6:27 PM Michael Platings
>> <Michael.Platings at arm.com<mailto: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 at google.com/T/
>> [3]
>>
>> FROM: Rui Ueyama <ruiu at google.com<mailto:ruiu at
google.com>>
>> SENT: 07 June 2019 08:42
>> TO: Chris Lattner <clattner at nondot.org<mailto:clattner at
nondot.org>>
>> CC: Michael Platings <Michael.Platings at
arm.com<mailto:Michael.Platings at arm.com>>;
>> llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>; nd
<nd at arm.com<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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 at 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<mailto: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/20190718/b7749208/attachment-0001.html>

Hi Denis,

On Fri, Jul 19, 2019 at 6:55 AM Bakhvalov, Denis <denis.bakhvalov at
intel.com>
wrote:
> Hello Rui, all,
>
> We recently went through the LLD renaming process at Intel and it went
> reasonably well too.
>
Very glad to hear that!

> However I encountered one case when the tool seems not to work:
>
>
>
> int foo()
>
> {
>
>   int A = 1;
>
>   int B = 2;
>
> #ifdef __WIN32
>
>   int C = A;
>
> #else
>
>   int C = B;
>
> #endif
>
>   return C;
>
> }
>
>
>
> After running the tool only one of #ifdef’ed branch will be renamed.
> Another will be left broken.
>
> Is there a plan how we will deal with this?
>
I'm planning to do something to deal with that, as this pattern does not
only occur in downstream repos but occurs in the main repository.

A clang-based tool works on an AST, so each run of a tool can see only one
part of `#ifdef ... #elif ... #else ... #endif` and fix only code that is
visible to the tool at that moment. That being said, if we are able to run
the tool multiple times in different configurations and merge the results
to a single commit, then we are able to make a single change that covers
all `#ifdef`s, given that you have environments to compile code for all the
targets to cover all `#ifdef`s. So I'm trying to make that doable.

> And one more question:
>
> What is the plan of renaming variables in llvm and clang? Will it be done
> gradually, i.e. directory by directory?
>
That's a big topic. I was thinking of starting a new thread just for that.
Originally I was trying to do this directory-by-directory basis, but now
I'm inclined to do this in a single extremely large patch. There's a
risk
of doing this that way (in particular, I'm not confident that I can keep
the buildbots green or will be able to make them green in a timely manner
after committing), but I think that will make downstream repo maintainer's
work easier.

>
> Best regards,
> Denis Bakhvalov.
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of
*Rui
> Ueyama via llvm-dev
> *Sent:* Monday, July 15, 2019 22:55
> *To:* Edd Dawson <edd-llvm at mr-edd.co.uk>
> *Cc:* llvm-dev <llvm-dev at lists.llvm.org>; nd <nd at arm.com>
> *Subject:* Re: [llvm-dev] RFC: changing variable naming rules in LLVM
> codebase
>
>
>
> 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 at
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 at
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/20190719/8063f1c8/attachment.html>