Alex Brachet-Mialot via llvm-dev
2019-Jul-10 11:12 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
Rui, I have created D64474 to change comments explicitly stating the parameter names for constants ( /*parameterName=*/<constant> ). I did this by hand to match the new variable names. Do you know if there would be a way to update these comments with a tool similar to what you used to change these names? Perhaps it would be much more difficult, I'm guessing clang's AST doesn't have a way to describe comments? It's obviously not a huge deal to have these changed it could be done over time. Best, Alex On Wed, Jul 10, 2019 at 12:18 AM Rui Ueyama via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hi Joan, > > On Tue, Jul 9, 2019 at 9:46 PM Joan Lluch <joan.lluch at icloud.com> wrote: > >> Hi Rui, >> >> I’m totally positive on switching to camelCase convention. In fact I have >> been always uncomfortable with the current naming approach. >> >> My only suggestion/concern is that we should provide a transition path >> not only for the trunk code in the repository, but for eventual >> out-of-trunk code with implementations of custom architectures. I have >> currently a custom backend implemented on top of LLVM 9 and therefore this >> change will surely break my code. I assume that developers affected by this >> will be able to run the converting tools to fix their own code. >> > > The tool that I wrote for lld's style conversion should work for > out-of-trunk code, so as I described in the previous email, third-party > code maintainer should be able to use the tool to convert the style first > in their repositories and then rebase in order to avoid large merge > conflicts. > > The tool needs to be polished to convert other subprojects such as clang, > but I'll keep your request in mind. I'll try my best to provide a smooth > transition path for out-of-trunk code for any change that I'll submit for > the style change. > > John >> >> On 9 Jul 2019, at 09:23, Rui Ueyama via llvm-dev <llvm-dev at lists.llvm.org> >> wrote: >> >> Thanks, Chris. >> >> Looks like everybody is at least mildly comfortable with my variable name >> renaming change, so I'll to submit that change to lld subdirectory soon. If >> you have any objections, please let me know. Note that this is not the end >> of my effort but actually the beginning of it. As Chris said, I believe we >> should do this to the entire LLVM codebase. I'm planning to do that >> directory-by-directory basis. >> >> On Tue, Jul 9, 2019 at 2:03 PM Chris Lattner <clattner at nondot.org> wrote: >> >>> This looks really great to me Rui, and I’m also pleased to see the >>> positive comments on the review thread. Thank you for driving this forward! >>> >>> -Chris >>> >>> >>> On Jul 4, 2019, at 9:50 PM, Rui Ueyama <ruiu at google.com> wrote: >>> >>> Hi all, >>> >>> I wrote a tool <https://reviews.llvm.org/D64123> to batch-rename >>> variable names so that they are in camelCase, and I applied the tool to lld >>> subdirectory. You can see my change at https://reviews.llvm.org/D64121. >>> If you have any comments, please reply. >>> >>> If people are happy about this change, I can do the same thing for other >>> directories including LLVM itself and Clang. >>> >>> On Mon, Jun 10, 2019 at 6:34 PM Rui Ueyama <ruiu at google.com> wrote: >>> >>>> On Mon, Jun 10, 2019 at 6:27 PM Michael Platings < >>>> Michael.Platings at arm.com> wrote: >>>> >>>>> Hi Rui, >>>>> >>>>> >>>>> >>>>> As per the provisional plan [1] we’re still at step 1: improving git >>>>> blame. The status of this is that there are some fairly mature patches in >>>>> the Git project’s queue [2], and I’m hopeful that it will be accepted in >>>>> something close to its current form. >>>>> >>>>> >>>>> >>>>> But if you can get started on steps 2 & 3 i.e. making forks available >>>>> with the possible changes applied then that would be great. My hope is that >>>>> once everyone can see what the options really look like then it will be >>>>> easier to reach consensus. >>>>> >>>> >>>> Sure, I'll try to do that. I'll probably start with finding identifiers >>>> and typenames that will conflict after the naming scheme change and rename >>>> them so that they won't conflict. The number of such symbols would >>>> hopefully be small, and submitting such renaming changes wouldn't be >>>> distracting. After that, I think I can create a mechanical change to rename >>>> variables to see how it will look like. >>>> >>>> >>>>> >>>>> Thanks, >>>>> >>>>> -Michael >>>>> >>>>> >>>>> >>>>> [1] >>>>> https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan >>>>> >>>>> [2] >>>>> https://public-inbox.org/git/20190515214503.77162-8-brho at google.com/T/ >>>>> >>>>> >>>>> >>>>> *From:* Rui Ueyama <ruiu at google.com> >>>>> *Sent:* 07 June 2019 08:42 >>>>> *To:* Chris Lattner <clattner at nondot.org> >>>>> *Cc:* Michael Platings <Michael.Platings at arm.com>; >>>>> llvm-dev at lists.llvm.org; nd <nd at arm.com> >>>>> *Subject:* Re: [llvm-dev] RFC: changing variable naming rules in LLVM >>>>> codebase >>>>> >>>>> >>>>> >>>>> This thread is not active for a while, but I'm still interested in >>>>> seeing a change. >>>>> >>>>> >>>>> >>>>> Reading through this thread, looks like we can agree that we want to >>>>> change the local variable naming scheme so that they start with a lowercase >>>>> letter. Besides that, there were discussions about lower_case, camelCase, >>>>> m_ prefix, and each argument seems as convincing as others. I'm not sure >>>>> what is the decision making process in a situation like this. >>>>> >>>>> >>>>> >>>>> I'd personally vote for changing local variables to start with a >>>>> lowercase letter and keep other naming conventions as-is to make it a >>>>> minimum change. >>>>> >>>>> >>>>> >>>>> As I stated before, I'm happy to make a change to lld to see how a >>>>> naming convention change will look like, but in order to do that I need to >>>>> get at least a rough consensus to do that. What is a way to proceed? >>>>> >>>>> >>>>> >>>>> On Sat, May 25, 2019 at 3:00 PM Chris Lattner via llvm-dev < >>>>> llvm-dev at lists.llvm.org> wrote: >>>>> >>>>> Hi Michael, >>>>> >>>>> >>>>> >>>>> I’m still very interested in seeing a change here. If someone is >>>>> interested in seeing a codebase using the proposed camelBack convention for >>>>> variables names, the MLIR codebase is public >>>>> <https://github.com/tensorflow/mlir> and uses it. >>>>> >>>>> >>>>> >>>>> If you’re curious to see what this looks like in practice, there are >>>>> lots of examples in the codebase, here is an example .cpp file >>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Transforms/LoopUnrollAndJam.cpp> >>>>> , here is another >>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Parser/Parser.cpp>, >>>>> here is an example header >>>>> <https://github.com/tensorflow/mlir/blob/master/include/mlir/IR/Location.h> >>>>> . >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> We are still working our way through open sourcing logistics (not all >>>>> the code is out yet), but we would still like to contribute at least parts >>>>> of this to LLVM if the project is interested. [[This is just an FYI, not >>>>> itself a proposal yet - one will be coming when we’re ready.]] >>>>> >>>>> >>>>> >>>>> -Chris >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On May 21, 2019, at 3:01 AM, Michael Platings via llvm-dev < >>>>> llvm-dev at lists.llvm.org> wrote: >>>>> >>>>> >>>>> >>>>> Hi folks, >>>>> >>>>> Git is on its way to learning how to ignore commits, allowing us to do >>>>> variable renaming and other small refactorings without breaking git blame. >>>>> >>>>> It's like git-hyper-blame [1] but significantly more powerful as it >>>>> uses fuzzy matching to match lines, including lines that may have been >>>>> split or joined. >>>>> >>>>> A preview release of Git with this new feature is at: >>>>> https://github.com/mplatings/git/releases/tag/ignore-rev >>>>> >>>>> Some of you have told me that you already have to spend time running >>>>> git blame multiple times to look past uninteresting commits so I'd love for >>>>> you to give this feature a try and see if it helps you. Your feedback will >>>>> be very valuable. >>>>> >>>>> Thanks, >>>>> -Michael >>>>> >>>>> [1] >>>>> https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html >>>>> _______________________________________________ >>>>> LLVM Developers mailing list >>>>> llvm-dev at lists.llvm.org >>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> LLVM Developers mailing list >>>>> llvm-dev at lists.llvm.org >>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>> >>>>> >>> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190710/45068960/attachment-0001.html>
Rui Ueyama via llvm-dev
2019-Jul-10 11:20 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
Alex, Thank you for pointing that out. I haven't thought that before. Maybe it is not very easy to update a comment like that using Clang AST, but I guess that a simple Perl-style regex (such as `/*[A-Z])(.*)=*/.\1`) would work fine as the pattern is unique, and I don't think that there are too many false positives. Let me try. On Wed, Jul 10, 2019 at 8:12 PM Alex Brachet-Mialot < alexbrachetmialot at gmail.com> wrote:> Rui, > > I have created D64474 to change comments explicitly stating the parameter > names for constants ( /*parameterName=*/<constant> ). I did this by hand to > match the new variable names. Do you know if there would be a way to update > these comments with a tool similar to what you used to change these names? > Perhaps it would be much more difficult, I'm guessing clang's AST doesn't > have a way to describe comments? It's obviously not a huge deal to have > these changed it could be done over time. > > Best, > Alex > > On Wed, Jul 10, 2019 at 12:18 AM Rui Ueyama via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Hi Joan, >> >> On Tue, Jul 9, 2019 at 9:46 PM Joan Lluch <joan.lluch at icloud.com> wrote: >> >>> Hi Rui, >>> >>> I’m totally positive on switching to camelCase convention. In fact I >>> have been always uncomfortable with the current naming approach. >>> >>> My only suggestion/concern is that we should provide a transition path >>> not only for the trunk code in the repository, but for eventual >>> out-of-trunk code with implementations of custom architectures. I have >>> currently a custom backend implemented on top of LLVM 9 and therefore this >>> change will surely break my code. I assume that developers affected by this >>> will be able to run the converting tools to fix their own code. >>> >> >> The tool that I wrote for lld's style conversion should work for >> out-of-trunk code, so as I described in the previous email, third-party >> code maintainer should be able to use the tool to convert the style first >> in their repositories and then rebase in order to avoid large merge >> conflicts. >> >> The tool needs to be polished to convert other subprojects such as clang, >> but I'll keep your request in mind. I'll try my best to provide a smooth >> transition path for out-of-trunk code for any change that I'll submit for >> the style change. >> >> John >>> >>> On 9 Jul 2019, at 09:23, Rui Ueyama via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>> Thanks, Chris. >>> >>> Looks like everybody is at least mildly comfortable with my variable >>> name renaming change, so I'll to submit that change to lld subdirectory >>> soon. If you have any objections, please let me know. Note that this is not >>> the end of my effort but actually the beginning of it. As Chris said, I >>> believe we should do this to the entire LLVM codebase. I'm planning to do >>> that directory-by-directory basis. >>> >>> On Tue, Jul 9, 2019 at 2:03 PM Chris Lattner <clattner at nondot.org> >>> wrote: >>> >>>> This looks really great to me Rui, and I’m also pleased to see the >>>> positive comments on the review thread. Thank you for driving this forward! >>>> >>>> -Chris >>>> >>>> >>>> On Jul 4, 2019, at 9:50 PM, Rui Ueyama <ruiu at google.com> wrote: >>>> >>>> Hi all, >>>> >>>> I wrote a tool <https://reviews.llvm.org/D64123> to batch-rename >>>> variable names so that they are in camelCase, and I applied the tool to lld >>>> subdirectory. You can see my change at https://reviews.llvm.org/D64121. >>>> If you have any comments, please reply. >>>> >>>> If people are happy about this change, I can do the same thing for >>>> other directories including LLVM itself and Clang. >>>> >>>> On Mon, Jun 10, 2019 at 6:34 PM Rui Ueyama <ruiu at google.com> wrote: >>>> >>>>> On Mon, Jun 10, 2019 at 6:27 PM Michael Platings < >>>>> Michael.Platings at arm.com> wrote: >>>>> >>>>>> Hi Rui, >>>>>> >>>>>> >>>>>> >>>>>> As per the provisional plan [1] we’re still at step 1: improving git >>>>>> blame. The status of this is that there are some fairly mature patches in >>>>>> the Git project’s queue [2], and I’m hopeful that it will be accepted in >>>>>> something close to its current form. >>>>>> >>>>>> >>>>>> >>>>>> But if you can get started on steps 2 & 3 i.e. making forks available >>>>>> with the possible changes applied then that would be great. My hope is that >>>>>> once everyone can see what the options really look like then it will be >>>>>> easier to reach consensus. >>>>>> >>>>> >>>>> Sure, I'll try to do that. I'll probably start with finding >>>>> identifiers and typenames that will conflict after the naming scheme change >>>>> and rename them so that they won't conflict. The number of such symbols >>>>> would hopefully be small, and submitting such renaming changes wouldn't be >>>>> distracting. After that, I think I can create a mechanical change to rename >>>>> variables to see how it will look like. >>>>> >>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>> -Michael >>>>>> >>>>>> >>>>>> >>>>>> [1] >>>>>> https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan >>>>>> >>>>>> [2] >>>>>> https://public-inbox.org/git/20190515214503.77162-8-brho at google.com/T/ >>>>>> >>>>>> >>>>>> >>>>>> *From:* Rui Ueyama <ruiu at google.com> >>>>>> *Sent:* 07 June 2019 08:42 >>>>>> *To:* Chris Lattner <clattner at nondot.org> >>>>>> *Cc:* Michael Platings <Michael.Platings at arm.com>; >>>>>> llvm-dev at lists.llvm.org; nd <nd at arm.com> >>>>>> *Subject:* Re: [llvm-dev] RFC: changing variable naming rules in >>>>>> LLVM codebase >>>>>> >>>>>> >>>>>> >>>>>> This thread is not active for a while, but I'm still interested in >>>>>> seeing a change. >>>>>> >>>>>> >>>>>> >>>>>> Reading through this thread, looks like we can agree that we want to >>>>>> change the local variable naming scheme so that they start with a lowercase >>>>>> letter. Besides that, there were discussions about lower_case, camelCase, >>>>>> m_ prefix, and each argument seems as convincing as others. I'm not sure >>>>>> what is the decision making process in a situation like this. >>>>>> >>>>>> >>>>>> >>>>>> I'd personally vote for changing local variables to start with a >>>>>> lowercase letter and keep other naming conventions as-is to make it a >>>>>> minimum change. >>>>>> >>>>>> >>>>>> >>>>>> As I stated before, I'm happy to make a change to lld to see how a >>>>>> naming convention change will look like, but in order to do that I need to >>>>>> get at least a rough consensus to do that. What is a way to proceed? >>>>>> >>>>>> >>>>>> >>>>>> On Sat, May 25, 2019 at 3:00 PM Chris Lattner via llvm-dev < >>>>>> llvm-dev at lists.llvm.org> wrote: >>>>>> >>>>>> Hi Michael, >>>>>> >>>>>> >>>>>> >>>>>> I’m still very interested in seeing a change here. If someone is >>>>>> interested in seeing a codebase using the proposed camelBack convention for >>>>>> variables names, the MLIR codebase is public >>>>>> <https://github.com/tensorflow/mlir> and uses it. >>>>>> >>>>>> >>>>>> >>>>>> If you’re curious to see what this looks like in practice, there are >>>>>> lots of examples in the codebase, here is an example .cpp file >>>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Transforms/LoopUnrollAndJam.cpp> >>>>>> , here is another >>>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Parser/Parser.cpp>, >>>>>> here is an example header >>>>>> <https://github.com/tensorflow/mlir/blob/master/include/mlir/IR/Location.h> >>>>>> . >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> We are still working our way through open sourcing logistics (not all >>>>>> the code is out yet), but we would still like to contribute at least parts >>>>>> of this to LLVM if the project is interested. [[This is just an FYI, not >>>>>> itself a proposal yet - one will be coming when we’re ready.]] >>>>>> >>>>>> >>>>>> >>>>>> -Chris >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On May 21, 2019, at 3:01 AM, Michael Platings via llvm-dev < >>>>>> llvm-dev at lists.llvm.org> wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi folks, >>>>>> >>>>>> Git is on its way to learning how to ignore commits, allowing us to >>>>>> do variable renaming and other small refactorings without breaking git >>>>>> blame. >>>>>> >>>>>> It's like git-hyper-blame [1] but significantly more powerful as it >>>>>> uses fuzzy matching to match lines, including lines that may have been >>>>>> split or joined. >>>>>> >>>>>> A preview release of Git with this new feature is at: >>>>>> https://github.com/mplatings/git/releases/tag/ignore-rev >>>>>> >>>>>> Some of you have told me that you already have to spend time running >>>>>> git blame multiple times to look past uninteresting commits so I'd love for >>>>>> you to give this feature a try and see if it helps you. Your feedback will >>>>>> be very valuable. >>>>>> >>>>>> Thanks, >>>>>> -Michael >>>>>> >>>>>> [1] >>>>>> https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html >>>>>> _______________________________________________ >>>>>> LLVM Developers mailing list >>>>>> llvm-dev at lists.llvm.org >>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> LLVM Developers mailing list >>>>>> llvm-dev at lists.llvm.org >>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>>> >>>>>> >>>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190710/4a414047/attachment.html>
Alex Brachet-Mialot via llvm-dev
2019-Jul-10 11:20 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
Also, now that I think about it, I believe doxygen will fail to build if the @parameter comments aren't changed to match the new names, my guess is it case sensitive. So perhaps we will need to find a way to manually change these names for doxygen comments? On Wed, Jul 10, 2019 at 7:12 AM Alex Brachet-Mialot < alexbrachetmialot at gmail.com> wrote:> Rui, > > I have created D64474 to change comments explicitly stating the parameter > names for constants ( /*parameterName=*/<constant> ). I did this by hand to > match the new variable names. Do you know if there would be a way to update > these comments with a tool similar to what you used to change these names? > Perhaps it would be much more difficult, I'm guessing clang's AST doesn't > have a way to describe comments? It's obviously not a huge deal to have > these changed it could be done over time. > > Best, > Alex > > On Wed, Jul 10, 2019 at 12:18 AM Rui Ueyama via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Hi Joan, >> >> On Tue, Jul 9, 2019 at 9:46 PM Joan Lluch <joan.lluch at icloud.com> wrote: >> >>> Hi Rui, >>> >>> I’m totally positive on switching to camelCase convention. In fact I >>> have been always uncomfortable with the current naming approach. >>> >>> My only suggestion/concern is that we should provide a transition path >>> not only for the trunk code in the repository, but for eventual >>> out-of-trunk code with implementations of custom architectures. I have >>> currently a custom backend implemented on top of LLVM 9 and therefore this >>> change will surely break my code. I assume that developers affected by this >>> will be able to run the converting tools to fix their own code. >>> >> >> The tool that I wrote for lld's style conversion should work for >> out-of-trunk code, so as I described in the previous email, third-party >> code maintainer should be able to use the tool to convert the style first >> in their repositories and then rebase in order to avoid large merge >> conflicts. >> >> The tool needs to be polished to convert other subprojects such as clang, >> but I'll keep your request in mind. I'll try my best to provide a smooth >> transition path for out-of-trunk code for any change that I'll submit for >> the style change. >> >> John >>> >>> On 9 Jul 2019, at 09:23, Rui Ueyama via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>> Thanks, Chris. >>> >>> Looks like everybody is at least mildly comfortable with my variable >>> name renaming change, so I'll to submit that change to lld subdirectory >>> soon. If you have any objections, please let me know. Note that this is not >>> the end of my effort but actually the beginning of it. As Chris said, I >>> believe we should do this to the entire LLVM codebase. I'm planning to do >>> that directory-by-directory basis. >>> >>> On Tue, Jul 9, 2019 at 2:03 PM Chris Lattner <clattner at nondot.org> >>> wrote: >>> >>>> This looks really great to me Rui, and I’m also pleased to see the >>>> positive comments on the review thread. Thank you for driving this forward! >>>> >>>> -Chris >>>> >>>> >>>> On Jul 4, 2019, at 9:50 PM, Rui Ueyama <ruiu at google.com> wrote: >>>> >>>> Hi all, >>>> >>>> I wrote a tool <https://reviews.llvm.org/D64123> to batch-rename >>>> variable names so that they are in camelCase, and I applied the tool to lld >>>> subdirectory. You can see my change at https://reviews.llvm.org/D64121. >>>> If you have any comments, please reply. >>>> >>>> If people are happy about this change, I can do the same thing for >>>> other directories including LLVM itself and Clang. >>>> >>>> On Mon, Jun 10, 2019 at 6:34 PM Rui Ueyama <ruiu at google.com> wrote: >>>> >>>>> On Mon, Jun 10, 2019 at 6:27 PM Michael Platings < >>>>> Michael.Platings at arm.com> wrote: >>>>> >>>>>> Hi Rui, >>>>>> >>>>>> >>>>>> >>>>>> As per the provisional plan [1] we’re still at step 1: improving git >>>>>> blame. The status of this is that there are some fairly mature patches in >>>>>> the Git project’s queue [2], and I’m hopeful that it will be accepted in >>>>>> something close to its current form. >>>>>> >>>>>> >>>>>> >>>>>> But if you can get started on steps 2 & 3 i.e. making forks available >>>>>> with the possible changes applied then that would be great. My hope is that >>>>>> once everyone can see what the options really look like then it will be >>>>>> easier to reach consensus. >>>>>> >>>>> >>>>> Sure, I'll try to do that. I'll probably start with finding >>>>> identifiers and typenames that will conflict after the naming scheme change >>>>> and rename them so that they won't conflict. The number of such symbols >>>>> would hopefully be small, and submitting such renaming changes wouldn't be >>>>> distracting. After that, I think I can create a mechanical change to rename >>>>> variables to see how it will look like. >>>>> >>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>> -Michael >>>>>> >>>>>> >>>>>> >>>>>> [1] >>>>>> https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan >>>>>> >>>>>> [2] >>>>>> https://public-inbox.org/git/20190515214503.77162-8-brho at google.com/T/ >>>>>> >>>>>> >>>>>> >>>>>> *From:* Rui Ueyama <ruiu at google.com> >>>>>> *Sent:* 07 June 2019 08:42 >>>>>> *To:* Chris Lattner <clattner at nondot.org> >>>>>> *Cc:* Michael Platings <Michael.Platings at arm.com>; >>>>>> llvm-dev at lists.llvm.org; nd <nd at arm.com> >>>>>> *Subject:* Re: [llvm-dev] RFC: changing variable naming rules in >>>>>> LLVM codebase >>>>>> >>>>>> >>>>>> >>>>>> This thread is not active for a while, but I'm still interested in >>>>>> seeing a change. >>>>>> >>>>>> >>>>>> >>>>>> Reading through this thread, looks like we can agree that we want to >>>>>> change the local variable naming scheme so that they start with a lowercase >>>>>> letter. Besides that, there were discussions about lower_case, camelCase, >>>>>> m_ prefix, and each argument seems as convincing as others. I'm not sure >>>>>> what is the decision making process in a situation like this. >>>>>> >>>>>> >>>>>> >>>>>> I'd personally vote for changing local variables to start with a >>>>>> lowercase letter and keep other naming conventions as-is to make it a >>>>>> minimum change. >>>>>> >>>>>> >>>>>> >>>>>> As I stated before, I'm happy to make a change to lld to see how a >>>>>> naming convention change will look like, but in order to do that I need to >>>>>> get at least a rough consensus to do that. What is a way to proceed? >>>>>> >>>>>> >>>>>> >>>>>> On Sat, May 25, 2019 at 3:00 PM Chris Lattner via llvm-dev < >>>>>> llvm-dev at lists.llvm.org> wrote: >>>>>> >>>>>> Hi Michael, >>>>>> >>>>>> >>>>>> >>>>>> I’m still very interested in seeing a change here. If someone is >>>>>> interested in seeing a codebase using the proposed camelBack convention for >>>>>> variables names, the MLIR codebase is public >>>>>> <https://github.com/tensorflow/mlir> and uses it. >>>>>> >>>>>> >>>>>> >>>>>> If you’re curious to see what this looks like in practice, there are >>>>>> lots of examples in the codebase, here is an example .cpp file >>>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Transforms/LoopUnrollAndJam.cpp> >>>>>> , here is another >>>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Parser/Parser.cpp>, >>>>>> here is an example header >>>>>> <https://github.com/tensorflow/mlir/blob/master/include/mlir/IR/Location.h> >>>>>> . >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> We are still working our way through open sourcing logistics (not all >>>>>> the code is out yet), but we would still like to contribute at least parts >>>>>> of this to LLVM if the project is interested. [[This is just an FYI, not >>>>>> itself a proposal yet - one will be coming when we’re ready.]] >>>>>> >>>>>> >>>>>> >>>>>> -Chris >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On May 21, 2019, at 3:01 AM, Michael Platings via llvm-dev < >>>>>> llvm-dev at lists.llvm.org> wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi folks, >>>>>> >>>>>> Git is on its way to learning how to ignore commits, allowing us to >>>>>> do variable renaming and other small refactorings without breaking git >>>>>> blame. >>>>>> >>>>>> It's like git-hyper-blame [1] but significantly more powerful as it >>>>>> uses fuzzy matching to match lines, including lines that may have been >>>>>> split or joined. >>>>>> >>>>>> A preview release of Git with this new feature is at: >>>>>> https://github.com/mplatings/git/releases/tag/ignore-rev >>>>>> >>>>>> Some of you have told me that you already have to spend time running >>>>>> git blame multiple times to look past uninteresting commits so I'd love for >>>>>> you to give this feature a try and see if it helps you. Your feedback will >>>>>> be very valuable. >>>>>> >>>>>> Thanks, >>>>>> -Michael >>>>>> >>>>>> [1] >>>>>> https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html >>>>>> _______________________________________________ >>>>>> LLVM Developers mailing list >>>>>> llvm-dev at lists.llvm.org >>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> LLVM Developers mailing list >>>>>> llvm-dev at lists.llvm.org >>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>>> >>>>>> >>>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190710/a4ad85f7/attachment.html>
Rui Ueyama via llvm-dev
2019-Jul-10 11:24 UTC
[llvm-dev] RFC: changing variable naming rules in LLVM codebase
Good point, too. I believe I can find lines starting with `@parameter` and apply the same name conversion rules to identifiers after `@parameter`. Since lld doesn't use doxygen comments, it is fine for now, but before moving forward, I'll address that. Thank you for pointing that out. On Wed, Jul 10, 2019 at 8:20 PM Alex Brachet-Mialot < alexbrachetmialot at gmail.com> wrote:> Also, now that I think about it, I believe doxygen will fail to build if > the @parameter comments aren't changed to match the new names, my guess is > it case sensitive. So perhaps we will need to find a way to manually change > these names for doxygen comments? > > On Wed, Jul 10, 2019 at 7:12 AM Alex Brachet-Mialot < > alexbrachetmialot at gmail.com> wrote: > >> Rui, >> >> I have created D64474 to change comments explicitly stating the parameter >> names for constants ( /*parameterName=*/<constant> ). I did this by hand to >> match the new variable names. Do you know if there would be a way to update >> these comments with a tool similar to what you used to change these names? >> Perhaps it would be much more difficult, I'm guessing clang's AST doesn't >> have a way to describe comments? It's obviously not a huge deal to have >> these changed it could be done over time. >> >> Best, >> Alex >> >> On Wed, Jul 10, 2019 at 12:18 AM Rui Ueyama via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> Hi Joan, >>> >>> On Tue, Jul 9, 2019 at 9:46 PM Joan Lluch <joan.lluch at icloud.com> wrote: >>> >>>> Hi Rui, >>>> >>>> I’m totally positive on switching to camelCase convention. In fact I >>>> have been always uncomfortable with the current naming approach. >>>> >>>> My only suggestion/concern is that we should provide a transition path >>>> not only for the trunk code in the repository, but for eventual >>>> out-of-trunk code with implementations of custom architectures. I have >>>> currently a custom backend implemented on top of LLVM 9 and therefore this >>>> change will surely break my code. I assume that developers affected by this >>>> will be able to run the converting tools to fix their own code. >>>> >>> >>> The tool that I wrote for lld's style conversion should work for >>> out-of-trunk code, so as I described in the previous email, third-party >>> code maintainer should be able to use the tool to convert the style first >>> in their repositories and then rebase in order to avoid large merge >>> conflicts. >>> >>> The tool needs to be polished to convert other subprojects such as >>> clang, but I'll keep your request in mind. I'll try my best to provide a >>> smooth transition path for out-of-trunk code for any change that I'll >>> submit for the style change. >>> >>> John >>>> >>>> On 9 Jul 2019, at 09:23, Rui Ueyama via llvm-dev < >>>> llvm-dev at lists.llvm.org> wrote: >>>> >>>> Thanks, Chris. >>>> >>>> Looks like everybody is at least mildly comfortable with my variable >>>> name renaming change, so I'll to submit that change to lld subdirectory >>>> soon. If you have any objections, please let me know. Note that this is not >>>> the end of my effort but actually the beginning of it. As Chris said, I >>>> believe we should do this to the entire LLVM codebase. I'm planning to do >>>> that directory-by-directory basis. >>>> >>>> On Tue, Jul 9, 2019 at 2:03 PM Chris Lattner <clattner at nondot.org> >>>> wrote: >>>> >>>>> This looks really great to me Rui, and I’m also pleased to see the >>>>> positive comments on the review thread. Thank you for driving this forward! >>>>> >>>>> -Chris >>>>> >>>>> >>>>> On Jul 4, 2019, at 9:50 PM, Rui Ueyama <ruiu at google.com> wrote: >>>>> >>>>> Hi all, >>>>> >>>>> I wrote a tool <https://reviews.llvm.org/D64123> to batch-rename >>>>> variable names so that they are in camelCase, and I applied the tool to lld >>>>> subdirectory. You can see my change at https://reviews.llvm.org/D64121. >>>>> If you have any comments, please reply. >>>>> >>>>> If people are happy about this change, I can do the same thing for >>>>> other directories including LLVM itself and Clang. >>>>> >>>>> On Mon, Jun 10, 2019 at 6:34 PM Rui Ueyama <ruiu at google.com> wrote: >>>>> >>>>>> On Mon, Jun 10, 2019 at 6:27 PM Michael Platings < >>>>>> Michael.Platings at arm.com> wrote: >>>>>> >>>>>>> Hi Rui, >>>>>>> >>>>>>> >>>>>>> >>>>>>> As per the provisional plan [1] we’re still at step 1: improving git >>>>>>> blame. The status of this is that there are some fairly mature patches in >>>>>>> the Git project’s queue [2], and I’m hopeful that it will be accepted in >>>>>>> something close to its current form. >>>>>>> >>>>>>> >>>>>>> >>>>>>> But if you can get started on steps 2 & 3 i.e. making forks >>>>>>> available with the possible changes applied then that would be great. My >>>>>>> hope is that once everyone can see what the options really look like then >>>>>>> it will be easier to reach consensus. >>>>>>> >>>>>> >>>>>> Sure, I'll try to do that. I'll probably start with finding >>>>>> identifiers and typenames that will conflict after the naming scheme change >>>>>> and rename them so that they won't conflict. The number of such symbols >>>>>> would hopefully be small, and submitting such renaming changes wouldn't be >>>>>> distracting. After that, I think I can create a mechanical change to rename >>>>>> variables to see how it will look like. >>>>>> >>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> -Michael >>>>>>> >>>>>>> >>>>>>> >>>>>>> [1] >>>>>>> https://llvm.org/docs/Proposals/VariableNames.html#provisional-plan >>>>>>> >>>>>>> [2] >>>>>>> https://public-inbox.org/git/20190515214503.77162-8-brho at google.com/T/ >>>>>>> >>>>>>> >>>>>>> >>>>>>> *From:* Rui Ueyama <ruiu at google.com> >>>>>>> *Sent:* 07 June 2019 08:42 >>>>>>> *To:* Chris Lattner <clattner at nondot.org> >>>>>>> *Cc:* Michael Platings <Michael.Platings at arm.com>; >>>>>>> llvm-dev at lists.llvm.org; nd <nd at arm.com> >>>>>>> *Subject:* Re: [llvm-dev] RFC: changing variable naming rules in >>>>>>> LLVM codebase >>>>>>> >>>>>>> >>>>>>> >>>>>>> This thread is not active for a while, but I'm still interested in >>>>>>> seeing a change. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Reading through this thread, looks like we can agree that we want to >>>>>>> change the local variable naming scheme so that they start with a lowercase >>>>>>> letter. Besides that, there were discussions about lower_case, camelCase, >>>>>>> m_ prefix, and each argument seems as convincing as others. I'm not sure >>>>>>> what is the decision making process in a situation like this. >>>>>>> >>>>>>> >>>>>>> >>>>>>> I'd personally vote for changing local variables to start with a >>>>>>> lowercase letter and keep other naming conventions as-is to make it a >>>>>>> minimum change. >>>>>>> >>>>>>> >>>>>>> >>>>>>> As I stated before, I'm happy to make a change to lld to see how a >>>>>>> naming convention change will look like, but in order to do that I need to >>>>>>> get at least a rough consensus to do that. What is a way to proceed? >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Sat, May 25, 2019 at 3:00 PM Chris Lattner via llvm-dev < >>>>>>> llvm-dev at lists.llvm.org> wrote: >>>>>>> >>>>>>> Hi Michael, >>>>>>> >>>>>>> >>>>>>> >>>>>>> I’m still very interested in seeing a change here. If someone is >>>>>>> interested in seeing a codebase using the proposed camelBack convention for >>>>>>> variables names, the MLIR codebase is public >>>>>>> <https://github.com/tensorflow/mlir> and uses it. >>>>>>> >>>>>>> >>>>>>> >>>>>>> If you’re curious to see what this looks like in practice, there are >>>>>>> lots of examples in the codebase, here is an example .cpp file >>>>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Transforms/LoopUnrollAndJam.cpp> >>>>>>> , here is another >>>>>>> <https://github.com/tensorflow/mlir/blob/master/lib/Parser/Parser.cpp>, >>>>>>> here is an example header >>>>>>> <https://github.com/tensorflow/mlir/blob/master/include/mlir/IR/Location.h> >>>>>>> . >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> We are still working our way through open sourcing logistics (not >>>>>>> all the code is out yet), but we would still like to contribute at least >>>>>>> parts of this to LLVM if the project is interested. [[This is just an FYI, >>>>>>> not itself a proposal yet - one will be coming when we’re ready.]] >>>>>>> >>>>>>> >>>>>>> >>>>>>> -Chris >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On May 21, 2019, at 3:01 AM, Michael Platings via llvm-dev < >>>>>>> llvm-dev at lists.llvm.org> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi folks, >>>>>>> >>>>>>> Git is on its way to learning how to ignore commits, allowing us to >>>>>>> do variable renaming and other small refactorings without breaking git >>>>>>> blame. >>>>>>> >>>>>>> It's like git-hyper-blame [1] but significantly more powerful as it >>>>>>> uses fuzzy matching to match lines, including lines that may have been >>>>>>> split or joined. >>>>>>> >>>>>>> A preview release of Git with this new feature is at: >>>>>>> https://github.com/mplatings/git/releases/tag/ignore-rev >>>>>>> >>>>>>> Some of you have told me that you already have to spend time running >>>>>>> git blame multiple times to look past uninteresting commits so I'd love for >>>>>>> you to give this feature a try and see if it helps you. Your feedback will >>>>>>> be very valuable. >>>>>>> >>>>>>> Thanks, >>>>>>> -Michael >>>>>>> >>>>>>> [1] >>>>>>> https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html >>>>>>> _______________________________________________ >>>>>>> LLVM Developers mailing list >>>>>>> llvm-dev at lists.llvm.org >>>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> LLVM Developers mailing list >>>>>>> llvm-dev at lists.llvm.org >>>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>>>> >>>>>>> >>>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> llvm-dev at lists.llvm.org >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> >>>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190710/81fb4ab6/attachment.html>