On Wed, Oct 8, 2014 at 7:20 PM, Nick Kledzik <kledzik at apple.com> wrote:> Sure, I actually have no problem with this. > > I'm going to point out that one of the naming conventions used by LLD has > serious problems: naming variables with a leading underscore puts them > *way* too close to the reserved identifier space. Folks have accidentally > ended up with reserved names quite a few times already. > > The lld conventions for ivars is a leading underscore followed by a > lowercase letter. The reserved identifiers are a leading underscore > followed by an uppercase letter. There is no conflict. > > On the other hand the LLVM conventions prevents the use of the -Wshadow > which catches real bugs. >If people think this is a valuable warning, we should just fix it so that it doesn't fire on this common pattern: struct A { A(int x) : x(x) {} int x; }; It should still fire on this pattern though: struct A { A(int x) : x(x) { x++; } // I modified the parameter 'x', not the member 'x'. int x; }; We shouldn't create conventions just to work around bugs in a toolchain that we control. --- My two cents: LLVM's conventions around method names and local variable names are widely ignored. For example, the IRBuilder uses leading capitals for methods. Clang CodeGen does as well. Clang CodeGen also uses lots of local variables with leading lower-case letters, which is against the variable naming guidelines. I think the takeaway from that is that lots of people don't actually understand or agree with the LLVM naming standards in this area. I don't think we should do a mass rename in LLD if we aren't willing to do a mass rename across Clang internals, and so far no one has indicated a desire to do that. If we limit the scope of the proposed change to removing leading underscores from fields, then that might be worth doing, but it's really up to you folks. I just don't think we're ready for a function and variable case unification yet. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141009/e6e8ab47/attachment.html>
On 9 October 2014 18:47, Reid Kleckner <rnk at google.com> wrote:> We shouldn't create conventions just to work around bugs in a toolchain that > we control.Good point.> I think the takeaway from that is that lots of people don't actually > understand or agree with the LLVM naming standards in this area. I don't > think we should do a mass rename in LLD if we aren't willing to do a mass > rename across Clang internals, and so far no one has indicated a desire to > do that.Another good point. I don't think we should do a mass renaming anywhere. If I'm not mistaken, the current state is mixed because we decided long ago to slowly refactor things as we go. I don't have the email thread where people agreed to that, nor I think there is one, but it used to be the argument against patches with different code style in the past, at least in my terrible memory.> If we limit the scope of the proposed change to removing leading underscores > from fields, then that might be worth doing, but it's really up to you > folks. I just don't think we're ready for a function and variable case > unification yet.Yet another... Focus on the problem, fix the toolchain and the source to overcome the problem and the rest can be done at the pace that we're doing the rest. cheers, --renato
On Oct 9, 2014, at 10:47 AM, Reid Kleckner <rnk at google.com> wrote:> On Wed, Oct 8, 2014 at 7:20 PM, Nick Kledzik <kledzik at apple.com> wrote: >> Sure, I actually have no problem with this. >> >> I'm going to point out that one of the naming conventions used by LLD has serious problems: naming variables with a leading underscore puts them *way* too close to the reserved identifier space. Folks have accidentally ended up with reserved names quite a few times already. > > The lld conventions for ivars is a leading underscore followed by a lowercase letter. The reserved identifiers are a leading underscore followed by an uppercase letter. There is no conflict. > > On the other hand the LLVM conventions prevents the use of the -Wshadow which catches real bugs. > > If people think this is a valuable warning, we should just fix it so that it doesn't fire on this common pattern: > struct A { > A(int x) : x(x) {} > int x; > }; > > It should still fire on this pattern though: > struct A { > A(int x) : x(x) { x++; } // I modified the parameter 'x', not the member 'x'. > int x; > }; > > We shouldn't create conventions just to work around bugs in a toolchain that we control.Or if your convention does not work with existing tools (clang, gcc, VisualStudio, etc), maybe that should be a hint that the convention is buggy.> My two cents: LLVM's conventions around method names and local variable names are widely ignored. For example, the IRBuilder uses leading capitals for methods. Clang CodeGen does as well. Clang CodeGen also uses lots of local variables with leading lower-case letters, which is against the variable naming guidelines. > > I think the takeaway from that is that lots of people don't actually understand or agree with the LLVM naming standards in this area. I don't think we should do a mass rename in LLD if we aren't willing to do a mass rename across Clang internals, and so far no one has indicated a desire to do that.Yes. The top of the LLVM coding conventions, the “golden rule” is don’t do mass renames. So, I’m surprised at the desire for mass renamings. But, as you said, there is lots of straying from the naming conventions. So, as Chris suggested, I’ll start a thread to discuss naming conventions. -Nick -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141009/e5ade0e0/attachment.html>
On Thu, Oct 9, 2014 at 2:58 PM, Nick Kledzik <kledzik at apple.com> wrote:> > On Oct 9, 2014, at 10:47 AM, Reid Kleckner <rnk at google.com> wrote: > > On Wed, Oct 8, 2014 at 7:20 PM, Nick Kledzik <kledzik at apple.com> wrote: > >> Sure, I actually have no problem with this. >> >> I'm going to point out that one of the naming conventions used by LLD has >> serious problems: naming variables with a leading underscore puts them >> *way* too close to the reserved identifier space. Folks have accidentally >> ended up with reserved names quite a few times already. >> >> The lld conventions for ivars is a leading underscore followed by a >> lowercase letter. The reserved identifiers are a leading underscore >> followed by an uppercase letter. There is no conflict. >> >> On the other hand the LLVM conventions prevents the use of the -Wshadow >> which catches real bugs. >> > > If people think this is a valuable warning, we should just fix it so that > it doesn't fire on this common pattern: > struct A { > A(int x) : x(x) {} > int x; > }; > > It should still fire on this pattern though: > struct A { > A(int x) : x(x) { x++; } // I modified the parameter 'x', not the member > 'x'. > int x; > }; > > We shouldn't create conventions just to work around bugs in a toolchain > that we control. > > Or if your convention does not work with existing tools (clang, gcc, > VisualStudio, etc), maybe that should be a hint that the convention is > buggy. >I don't really agree with this. The idiom of naming ctor parameters the same as members isn't uncommon - one reason -Wshadow isn't on by default, because large codebases don't conform to it so the signal/noise ratio is low. There are quite a few cases where we improve Clang based on usage in LLVM, or disable VS warnings rather than working around them in LLVM code.> > > > My two cents: LLVM's conventions around method names and local variable > names are widely ignored. For example, the IRBuilder uses leading capitals > for methods. Clang CodeGen does as well. Clang CodeGen also uses lots of > local variables with leading lower-case letters, which is against the > variable naming guidelines. > > I think the takeaway from that is that lots of people don't actually > understand or agree with the LLVM naming standards in this area. I don't > think we should do a mass rename in LLD if we aren't willing to do a mass > rename across Clang internals, and so far no one has indicated a desire to > do that. > > Yes. The top of the LLVM coding conventions, the “golden rule” is don’t > do mass renames. So, I’m surprised at the desire for mass renamings. > > But, as you said, there is lots of straying from the naming conventions. >So far as I know it's legacy (we didn't do a mass rename, so there's lots of old code still using prior conventions), consistency (use the local convention or at least change a whole file/class together - sometimes hard to get the momentum for that), or just simple mistakes (due to the inconsistency it's sometimes hard to remember the conventions), not deliberately going against the style conventions.> So, as Chris suggested, I’ll start a thread to discuss naming conventions. > > -Nick > > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141009/4ab15617/attachment.html>
On Oct 9, 2014, at 2:58 PM, Nick Kledzik <kledzik at apple.com> wrote:>> >> My two cents: LLVM's conventions around method names and local variable names are widely ignored. For example, the IRBuilder uses leading capitals for methods. Clang CodeGen does as well. Clang CodeGen also uses lots of local variables with leading lower-case letters, which is against the variable naming guidelines. >> >> I think the takeaway from that is that lots of people don't actually understand or agree with the LLVM naming standards in this area. I don't think we should do a mass rename in LLD if we aren't willing to do a mass rename across Clang internals, and so far no one has indicated a desire to do that. > Yes. The top of the LLVM coding conventions, the “golden rule” is don’t do mass renames. So, I’m surprised at the desire for mass renamings.I think we should separate the discussions of “what the right thing is” vs “what to do about it”. If we come to agreement about what the right thing is, we can figure out if incremental or big-bang is the right way to go. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141009/032031df/attachment.html>