[llvm-dev] RFC: changing variable naming rules in LLVM codebase

Mon Feb 25 02:35:46 PST 2019

On Fri, 22 Feb 2019 at 20:46, via llvm-dev <llvm-dev at lists.llvm.org> wrote:

>
>
> > -----Original Message-----
> > From: David Greene [mailto:dag at cray.com]
> > Sent: Friday, February 22, 2019 2:40 PM
> > To: Robinson, Paul
> > Cc: asb at lowrisc.org; clattner at nondot.org; llvm-dev at lists.llvm.org;
> > nd at arm.com
> > Subject: Re: [llvm-dev] RFC: changing variable naming rules in LLVM
> > codebase
> >
> > via llvm-dev <llvm-dev at lists.llvm.org> writes:
> >
> > > Looking at names of "variables" there's reasonable support for making
> > > them visually more distinct from other kinds of names.  Regarding
> > > making data-member names distinct from local variables, some people
> > > don't see why it should matter, others find it helpful; given this
> > > neutral-to-helpful spectrum, going with the kind-of helpful convention
> > > seems preferable.
> >
> > There are drawbacks to the "m_" prefix notation, though.  It makes it
> > more work to move entities between class and local scope.  It's extra
> > typing, it's hard to pronounce, etc.
>
> IMO these are pretty feeble objections. The "m_" is silent, for example.
> Moving entities between class and local scope isn't all that common, and
> the small bit of extra typing provides clarity to your colleagues who
> like to know what data is ephemeral and what is persistent.  It's equally
> true that using real words instead of shorthand abbreviations is more
> typing, but we accept that for the clarity it brings.
>

You might treat "m_" as silent, but I don't. It's just not how my mind
works when reading code. As for moving entities between class and local
scope - I've found myself regularly going from local scope to class scope
in the past in other projects at least, although I can't say the same for
LLVM. I do know I'd get annoyed by typing m_* before every member variable
I have to write, whereas I don't for using real words, but I accept that
might just be me.

> > Now, these may be considered trivial complaints, but given that some of
> > them have already been raised, I think we need some discussion about it.
> > If there's overwhelming support for "m_" (and/or "g_" etc.) then fine,
> > but if there's about even opinions either way, we ought to go with the
> > status quo, at least for now.
>
> I don't think "overwhelming" is an appropriate barrier to adoption. I
> hear people say "yes, this would make it easier for me to read the code"
> and on the other side "meh, I don't see the benefit"; however I do *not*
> see anyone saying "No, this really would interfere with my code-reading."
>
> >
> > > So:
> > >
> > > - Local variables and formal parameters should be lower_case, with
> > >   one exception: Variables/parameters that have lambda/function
> > >   type should follow the function-name spelling rules.
> >
> > "lower_case" is a pretty drastic change from CamelCase and camelCase.
> > So far the only argument for it I've seen is, "lldb uses it all over the
> > place."  Having one subproject drive the standard for everything else
> > seems backward.  It's certainly possible I missed other opinions on this
> > though.
>
> You have. For example, here's a "significant improvement" comment:
> http://lists.llvm.org/pipermail/llvm-dev/2019-February/130214.html
>
> The debate suffers from being spread across multiple dev threads plus
> a Phabricator review.  Not optimal, but that's what it is.
>
> >
> > > - Initialisms and other abbreviations would be considered words for
> > >   this purpose, so we have names such as:
> > >     tli   // Local variable for TargetLoweringInfo
> > >     m_cgm // Data member for CodeGenModule
> >
> > I actually think we need something stronger.  Acronyms should be
> > discouraged unless it's absolutely clear what it is.  As has been
> > pointed out, "tli" means at least two common and very different things:
> > "TargetLoweringInfo" and "TargetLibraryInfo."
>
> That's a separate discussion and would be something to pursue regardless
> of the other spelling conventions. Let's not drag that in here.  My
> inclusion of this is based on "we find these things in names today, and
> they aren't 'words' so what do we do with them" not on any assessment
> that they are good or bad on their own merits.
>
> >
> > > - I don't have a good suggestion for file-static/global variables.
> > >   Some people have suggested a "g_" prefix for globals, or possibly
> > >   an "s_" prefix for class-static data.
> >
> > Is this really helpful or does it just make the code more difficult to
> > work with?  I don't know since I've never used such conventions before.
> > But I'm hesitant to introduce really strong binding of scopes to names
> > because it make refactoring more tedious/difficult.
>
> I have, but in a project that had rather more global data than is good
> for anyone. I put it out there for discussion, and thanks for doing
> exactly that!
>
> >
> > > Some people have worried that the churn will cause blame issues.
> > > I respectfully point out that in my own archaeology I have to deal
> > > with lots of clang-format/indentation/other random semantically
> > > meaningless refactoring, this is just one more.  Also the point is
> > > not to optimize for git-blame but to optimize for reading what is
> > > there at the moment.
> >
> > I want to call out that we already have a policy on this in the current
> > coding standards:
> >
> >   Our long term goal is for the entire codebase to follow the
> >   convention, but we explicitly do not want patches that do large-scale
> >   reformatting of existing code.
> >
> > I could argue things either way, but we should realize that a mechanical
> > reformatting goes explicitly against current stated (and presumably
> > previously-agreed-upon) policy.
> >
> >                                -David
>
> Right, but my impression is that people who lived through the last update
> to the spelling conventions are less enthused about a slow transition,
> this time around, and that bit of the convention would necessarily also
> be up for revision.
>
> --paulr
>
> _______________________________________________
> 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/20190225/02ad6c2a/attachment.html>