<div dir="ltr"><div dir="ltr">Hi Edd,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 12, 2019 at 10:04 PM Edd Dawson <<a href="mailto:edd-llvm@mr-edd.co.uk">edd-llvm@mr-edd.co.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Rui, all,<br>
<br>
Yesterday I brought the variable-renaming commits in to Sony's <br>
downstream LLD. We have a merge-based flow rather than continually <br>
rebasing our patch set, but it went reasonably smoothly nevertheless.<br></blockquote><div><br></div><div>I'm very glad to hear that!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The one snag I hit is that the tool initially missed variables mentioned <br>
in assert()s. I didn't put much time in to investigating this, but I <br>
presume it's because my compile_commands.json was build with assert()s <br>
disabled and so the names mentioned in the predicates were invisible to <br>
clang-llvm-rename. The result was that I ended up with something that <br>
built cleanly with NDEBUG, but not otherwise. I guess this is <br>
essentially the same as the #ifdef'd-out code issue you mentioned, but <br>
its effect is probably more widespread.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It was easily remedied by building in another configuration and <br>
reapplying to tool, but it's something others might want to watch out <br>
for.<br>
<br>
Thanks,<br>
Edd<br>
<br>
On 2019-07-12 06:05, Rui Ueyama via llvm-dev wrote:<br>
> So, I submitted a few patches to rename all variables in lld. If you<br>
> are interested in how it looks like, pick up any .cpp or .h file from<br>
> <a href="https://github.com/llvm/llvm-project/tree/master/lld" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/tree/master/lld</a>.<br>
> <br>
> I learned a few things by doing this which will help me do the same<br>
> thing to other LLVM (sub-)projects.<br>
> <br>
>  - Overall a batch tool to rename variables worked reasonably well, so<br>
> the coding style change is doable.<br>
> <br>
>  - There were a few classes that have a member variable Foo and a<br>
> function foo(), which would conflict after renaming. I rename<br>
> variables manually before renaming them. That's not a scalable<br>
> solution, though. For a larger codebase, I'd probably need to automate<br>
> it by, for example, renaming foo() to getFoo() if there's Foo variable<br>
> already.<br>
> <br>
>  - There were a few variables that would become a reserved word such<br>
> as `new` or `private` after renaming. I renamed them manually before<br>
> mass-renaming. For scalability, it probably have to be automated by<br>
> appending `_` at the end, for example.<br>
> <br>
>  - My clang-based tool didn't work for #ifdef'ed-out code, which<br>
> caused unexpected failures on buildbots after submitting. I don't know<br>
> how to fix the tool so that the tool can handle code containing<br>
> #ifdefs, but at least we need to keep it in mind so that we can check<br>
> it manually.<br>
> <br>
>  - LLVM's `/*foo=*/`-style comment to annotate function arguments need<br>
> to be handled automatically to make the tool scalable. So is the<br>
> doxygen @parameter.<br>
> <br>
>  - Since a variable rename change virtually touches every line of<br>
> codebase, that would cause massive merge conflicts to downstream repos<br>
> if we don't do anything to support them. We need to provide a tool and<br>
> guidance as to how to apply the tool to a out-of-tree repo so that a<br>
> renaming change is merged smoothly.<br>
> <br>
> On Wed, Jul 10, 2019 at 8:24 PM Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br>
> <br>
>> Good point, too. I believe I can find lines starting with<br>
>> `@parameter` and apply the same name conversion rules to identifiers<br>
>> after `@parameter`. Since lld doesn't use doxygen comments, it is<br>
>> fine for now, but before moving forward, I'll address that. Thank<br>
>> you for pointing that out.<br>
>> <br>
>> On Wed, Jul 10, 2019 at 8:20 PM Alex Brachet-Mialot<br>
>> <<a href="mailto:alexbrachetmialot@gmail.com" target="_blank">alexbrachetmialot@gmail.com</a>> wrote:<br>
>> <br>
>> Also, now that I think about it, I believe doxygen will fail to<br>
>> build if the @parameter comments aren't changed to match the new<br>
>> names, my guess is it case sensitive. So perhaps we will need to<br>
>> find a way to manually change these names for doxygen comments?<br>
>> <br>
>> On Wed, Jul 10, 2019 at 7:12 AM Alex Brachet-Mialot<br>
>> <<a href="mailto:alexbrachetmialot@gmail.com" target="_blank">alexbrachetmialot@gmail.com</a>> wrote:<br>
>> <br>
>> Rui,<br>
>> <br>
>> I have created D64474 to change comments explicitly stating the<br>
>> parameter names for constants ( /*parameterName=*/<constant> ). I<br>
>> did this by hand to match the new variable names. Do you know if<br>
>> there would be a way to update these comments with a tool similar to<br>
>> what you used to change these names? Perhaps it would be much more<br>
>> difficult, I'm guessing clang's AST doesn't have a way to describe<br>
>> comments? It's obviously not a huge deal to have these changed it<br>
>> could be done over time.<br>
>> <br>
>> Best,<br>
>> Alex<br>
>> <br>
>> On Wed, Jul 10, 2019 at 12:18 AM Rui Ueyama via llvm-dev<br>
>> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> <br>
>> Hi Joan,<br>
>> <br>
>> On Tue, Jul 9, 2019 at 9:46 PM Joan Lluch <<a href="mailto:joan.lluch@icloud.com" target="_blank">joan.lluch@icloud.com</a>><br>
>> wrote:<br>
>> <br>
>> Hi Rui,<br>
>> <br>
>> I’m totally positive on switching to camelCase convention. In fact<br>
>> I have been always uncomfortable with the current naming approach.<br>
>> <br>
>> My only suggestion/concern is that we should provide a transition<br>
>> path not only for the trunk code in the repository, but for eventual<br>
>> out-of-trunk code with implementations of custom architectures. I<br>
>> have currently a custom backend implemented on top of LLVM 9 and<br>
>> therefore this change will surely break my code. I assume that<br>
>> developers affected by this will be able to run the converting tools<br>
>> to fix their own code.<br>
>> <br>
>> The tool that I wrote for lld's style conversion should work for<br>
>> out-of-trunk code, so as I described in the previous email,<br>
>> third-party code maintainer should be able to use the tool to<br>
>> convert the style first in their repositories and then rebase in<br>
>> order to avoid large merge conflicts.<br>
>> <br>
>> The tool needs to be polished to convert other subprojects such as<br>
>> clang, but I'll keep your request in mind. I'll try my best to<br>
>> provide a smooth transition path for out-of-trunk code for any<br>
>> change that I'll submit for the style change.<br>
>> <br>
>> John<br>
>> <br>
>> On 9 Jul 2019, at 09:23, Rui Ueyama via llvm-dev<br>
>> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> <br>
>> Thanks, Chris.<br>
>> <br>
>> Looks like everybody is at least mildly comfortable with my variable<br>
>> name renaming change, so I'll to submit that change to lld<br>
>> subdirectory soon. If you have any objections, please let me know.<br>
>> Note that this is not the end of my effort but actually the<br>
>> beginning of it. As Chris said, I believe we should do this to the<br>
>> entire LLVM codebase. I'm planning to do that directory-by-directory<br>
>> basis.<br>
>> <br>
>> On Tue, Jul 9, 2019 at 2:03 PM Chris Lattner <<a href="mailto:clattner@nondot.org" target="_blank">clattner@nondot.org</a>><br>
>> wrote:<br>
>> <br>
>> This looks really great to me Rui, and I’m also pleased to see the<br>
>> positive comments on the review thread.  Thank you for driving this<br>
>> forward!<br>
>> <br>
>> -Chris<br>
>> <br>
>> On Jul 4, 2019, at 9:50 PM, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br>
>> <br>
>> Hi all,<br>
>> <br>
>> I wrote a tool [1] to batch-rename variable names so that they are<br>
>> in camelCase, and I applied the tool to lld subdirectory. You can<br>
>> see my change at <a href="https://reviews.llvm.org/D64121" rel="noreferrer" target="_blank">https://reviews.llvm.org/D64121</a>. If you have any<br>
>> comments, please reply.<br>
>> <br>
>> If people are happy about this change, I can do the same thing for<br>
>> other directories including LLVM itself and Clang.<br>
>> <br>
>> On Mon, Jun 10, 2019 at 6:34 PM Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br>
>> <br>
>> On Mon, Jun 10, 2019 at 6:27 PM Michael Platings<br>
>> <<a href="mailto:Michael.Platings@arm.com" target="_blank">Michael.Platings@arm.com</a>> wrote:<br>
>> <br>
>> Hi Rui,<br>
>> <br>
>> As per the provisional plan [1] we’re still at step 1: improving<br>
>> git blame. The status of this is that there are some fairly mature<br>
>> patches in the Git project’s queue [2], and I’m hopeful that it<br>
>> will be accepted in something close to its current form.<br>
>> <br>
>> But if you can get started on steps 2 & 3 i.e. making forks<br>
>> available with the possible changes applied then that would be<br>
>> great. My hope is that once everyone can see what the options really<br>
>> look like then it will be easier to reach consensus.<br>
>> <br>
>> Sure, I'll try to do that. I'll probably start with finding<br>
>> identifiers and typenames that will conflict after the naming scheme<br>
>> change and rename them so that they won't conflict. The number of<br>
>> such symbols would hopefully be small, and submitting such renaming<br>
>> changes wouldn't be distracting. After that, I think I can create a<br>
>> mechanical change to rename variables to see how it will look like.<br>
>> <br>
>> Thanks,<br>
>> <br>
>> -Michael<br>
>> <br>
>> [1]<br>
>> <a href="https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan" rel="noreferrer" target="_blank">https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan</a><br>
>> [2]<br>
>> <br>
>> [2]<br>
>> <br>
> <a href="https://public-inbox.org/git/20190515214503.77162-8-brho@google.com/T/" rel="noreferrer" target="_blank">https://public-inbox.org/git/20190515214503.77162-8-brho@google.com/T/</a><br>
>> [3]<br>
>> <br>
>> FROM: Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>><br>
>> SENT: 07 June 2019 08:42<br>
>> TO: Chris Lattner <<a href="mailto:clattner@nondot.org" target="_blank">clattner@nondot.org</a>><br>
>> CC: Michael Platings <<a href="mailto:Michael.Platings@arm.com" target="_blank">Michael.Platings@arm.com</a>>;<br>
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>; nd <<a href="mailto:nd@arm.com" target="_blank">nd@arm.com</a>><br>
>> SUBJECT: Re: [llvm-dev] RFC: changing variable naming rules in LLVM<br>
>> codebase<br>
>> <br>
>> This thread is not active for a while, but I'm still interested in<br>
>> seeing a change.<br>
>> <br>
>> Reading through this thread, looks like we can agree that we want to<br>
>> change the local variable naming scheme so that they start with a<br>
>> lowercase letter. Besides that, there were discussions about<br>
>> lower_case, camelCase, m_ prefix, and each argument seems as<br>
>> convincing as others. I'm not sure what is the decision making<br>
>> process in a situation like this.<br>
>> <br>
>> I'd personally vote for changing local variables to start with a<br>
>> lowercase letter and keep other naming conventions as-is to make it<br>
>> a minimum change.<br>
>> <br>
>> As I stated before, I'm happy to make a change to lld to see how a<br>
>> naming convention change will look like, but in order to do that I<br>
>> need to get at least a rough consensus to do that. What is a way to<br>
>> proceed?<br>
>> <br>
>> On Sat, May 25, 2019 at 3:00 PM Chris Lattner via llvm-dev<br>
>> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> <br>
>> Hi Michael,<br>
>> <br>
>> I’m still very interested in seeing a change here.  If someone is<br>
>> interested in seeing a codebase using the proposed camelBack<br>
>> convention for variables names, the MLIR codebase is public [4] and<br>
>> uses it.<br>
>> <br>
>> If you’re curious to see what this looks like in practice, there<br>
>> are lots of examples in the codebase, here is an example .cpp file<br>
>> [5], here is another [6], here is an example header [7].<br>
>> <br>
>> We are still working our way through open sourcing logistics (not<br>
>> all the code is out yet), but we would still like to contribute at<br>
>> least parts of this to LLVM if the project is interested.  [[This is<br>
>> just an FYI, not itself a proposal yet - one will be coming when<br>
>> we’re ready.]]<br>
>> <br>
>> -Chris<br>
>> <br>
>> On May 21, 2019, at 3:01 AM, Michael Platings via llvm-dev<br>
>> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> <br>
>> Hi folks,<br>
>> <br>
>> Git is on its way to learning how to ignore commits, allowing us to<br>
>> do variable renaming and other small refactorings without breaking<br>
>> git blame.<br>
>> <br>
>> It's like git-hyper-blame [1] but significantly more powerful as it<br>
>> uses fuzzy matching to match lines, including lines that may have<br>
>> been split or joined.<br>
>> <br>
>> A preview release of Git with this new feature is at:<br>
>> <a href="https://github.com/mplatings/git/releases/tag/ignore-rev" rel="noreferrer" target="_blank">https://github.com/mplatings/git/releases/tag/ignore-rev</a> [8]<br>
>> <br>
>> Some of you have told me that you already have to spend time running<br>
>> git blame multiple times to look past uninteresting commits so I'd<br>
>> love for you to give this feature a try and see if it helps you.<br>
>> Your feedback will be very valuable.<br>
>> <br>
>> Thanks,<br>
>> -Michael<br>
>> <br>
>> [1]<br>
>> <br>
> <a href="https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html" rel="noreferrer" target="_blank">https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html</a><br>
>> [9]<br>
>> _______________________________________________<br>
>> LLVM Developers mailing list<br>
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
>> <br>
>> _______________________________________________<br>
>> LLVM Developers mailing list<br>
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
> <br>
>> _______________________________________________<br>
>> LLVM Developers mailing list<br>
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
>  _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
> <br>
> <br>
> Links:<br>
> ------<br>
> [1] <a href="https://reviews.llvm.org/D64123" rel="noreferrer" target="_blank">https://reviews.llvm.org/D64123</a><br>
> [2] <a href="https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan" rel="noreferrer" target="_blank">https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan</a><br>
> [3] <br>
> <a href="https://public-inbox.org/git/20190515214503.77162-8-brho@google.com/T/" rel="noreferrer" target="_blank">https://public-inbox.org/git/20190515214503.77162-8-brho@google.com/T/</a><br>
> [4] <a href="https://github.com/tensorflow/mlir" rel="noreferrer" target="_blank">https://github.com/tensorflow/mlir</a><br>
> [5]<br>
> <a href="https://github.com/tensorflow/mlir/blob/master/lib/Transforms/LoopUnrollAndJam.cpp" rel="noreferrer" target="_blank">https://github.com/tensorflow/mlir/blob/master/lib/Transforms/LoopUnrollAndJam.cpp</a><br>
> [6] <br>
> <a href="https://github.com/tensorflow/mlir/blob/master/lib/Parser/Parser.cpp" rel="noreferrer" target="_blank">https://github.com/tensorflow/mlir/blob/master/lib/Parser/Parser.cpp</a><br>
> [7] <br>
> <a href="https://github.com/tensorflow/mlir/blob/master/include/mlir/IR/Location.h" rel="noreferrer" target="_blank">https://github.com/tensorflow/mlir/blob/master/include/mlir/IR/Location.h</a><br>
> [8] <a href="https://github.com/mplatings/git/releases/tag/ignore-rev" rel="noreferrer" target="_blank">https://github.com/mplatings/git/releases/tag/ignore-rev</a><br>
> [9]<br>
> <a href="https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html" rel="noreferrer" target="_blank">https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html</a><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
<br>
-- <br>
Edd Dawson<br>
SN Systems - Sony Interactive Entertainment<br>
<a href="http://www.snsystems.com" rel="noreferrer" target="_blank">http://www.snsystems.com</a><br>
</blockquote></div></div>