virtual functions -> CamelCase

[Thiago Farina]

Why people violate this rule so much? virtual function should be named
using CamelCase, not unix_hacker style. Can we stop doing this, and
enforce it somehow?

[James Robinson]

On Tue, Apr 24, 2012 at 6:02 PM, Thiago Farina <tfa...@chromium.org> wrote:

Why people violate this rule so much?

Probably because they personally hate you.

More seriously, probably because it's easy to do and there's no tool or strong style guide reason not to.

 

virtual function should be named
using CamelCase, not unix_hacker style. Can we stop doing this, and
enforce it somehow?

Please read the appropriate section of the style guide: "Google style strongly encourages (but does not mandate) unix_hacker style on member functions that do so little work they're effectively free, such as simple, non-virtual getters and setters. This is also the team position. Some code uses CamelCase for everything; avoid writing patches just to convert these functions to unix_hacker style, but try to use it in new code you write unless there are encapsulation reasons why CamelCase would be much more appropriate." and think about what it means for what you are proposing.  In particular I think enforcing this with a tool would be a bad idea if there are a lot of existing violations (as you assert that there are) since it would mean lots of tool spew or lots of harmful churn to "fix" existing code.

- James

 

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Matt Perry]

On Tue, Apr 24, 2012 at 6:05 PM, James Robinson <jam...@google.com> wrote:

virtual function should be named
using CamelCase, not unix_hacker style. Can we stop doing this, and
enforce it somehow?

Please read the appropriate section of the style guide

Technically, a more appropriate section would be: 

• Simple setters and getters should generally be the only inline functions. These should be unix_hacker_style() (naming like a variable indicates they are as cheap to call as a variable is to access).  You're not inlining these because you want to speed things up, you're inlining them because it makes more sense than not inlining them, since they're just a direct member access anyway.  Note that virtual functions should never be declared this way as they are not truly as cheap as a variable access.

[Thiago Farina]

Yes! +1 for this wording.

--
Thiago

[Drew Wilson]

I suspect I've been the culprit at least once, but probably not because I personally hate Thiago. Typically what happens is we have some getter for an internal class state which uses unix_hacker style. Then I realize we want to mock it out in a test, so I make it virtual, without realizing I'm introducing a style violation.

Thanks for bringing it up - I shall go forth and sin no more!

-atw

On Tue, Apr 24, 2012 at 6:05 PM, James Robinson <jam...@google.com> wrote:

[Darin Fisher]

There was also at some point a big sweep of the codebase to de-inline a bunch of functions.  I believe this was done to improve build time by reducing dependencies in header files.  Perhaps it was too big of a change to also rename every affected function :-/

-Darin

[Jói Sigurðsson]

In the content refactoring, many hacker_style() accessors changed to
GetHackerStyle() accessors.

Working on content made me really, really, really hate this part of
our style guide.

I'd much rather have consistent CamelCase naming that doesn't have to
be changed when we refactor things, make things virtual to allow
overriding them, change a trivial function to be less trivial (e.g. to
cache a variable) and so forth.

Cheers,
Jói

[Peter Kasting]

On Wed, Apr 25, 2012 at 3:44 AM, Jói Sigurðsson <j...@chromium.org> wrote:

Working on content made me really, really, really hate this part of
our style guide.

I'd much rather have consistent CamelCase naming that doesn't have to
be changed when we refactor things, make things virtual to allow
overriding them, change a trivial function to be less trivial (e.g. to
cache a variable) and so forth.

It all depends what you're optimizing for.

I agree that the naming thing sucks when trying to modify formerly-simple code to become less-simple (e.g. making it virtual).  OTOH, I really appreciate writing code that calls classes with unix_hacker() accessors because it's very obvious when reading the caller-side code what's cheap, without having to constantly cross-reference the original class.

Personally I think the current way is the better tradeoff because all accessors are (hopefully) called somewhere but most will presumably not be refactored to need CamelCase later.  But I completely understand why you found this really painful.

As for the rest of the thread, I agree with Matt's wording.  I have tried in code reviews to enforce a similar policy.

PK

[Peter Kasting]

On Wed, Apr 25, 2012 at 11:02 AM, Andrew Wilson <atwi...@google.com> wrote:

BTW, can anyone actually think of a case where they would've written their code differently if a member function used CamelCase instead of linux_hacker style?

Well, the obvious one is that I always try to declare local variables to hold the results of CamelCase functions called more than once as long as the results should be constant, whereas with unix_hacker functions I often just call the function again.

I don't know about larger architectural decisions.  I think I may have made a couple of those, but they seem much rarer.

I think some of my appreciation for unix_hacker functions comes from confusion when reading the views codebase, which originally used CamelCase for pretty much everything, and I found myself constantly wondering "why is THAT expensive?  Is it doing some sort of chained lookup on the... oh wait, I guess it's not really expensive, they were just lying to me."

PK

[Drew Wilson]

Sending from the right address this time...

BTW, can anyone actually think of a case where they would've written their code differently if a member function used CamelCase instead of linux_hacker style? Because it seems to me that it's far better for a maintenance standpoint for us to use CamelCase everywhere, and if you're somehow in a tight loop where performance is important, you can take steps when writing that tight loop to address performance concerns (cache the results of the member function, etc).

Otherwise, if I need to change the implementation of a linux_hacker() function, not only do I need to change the name to CamelCase and update all of the callers, but I have to understand every single callsite to know whether it's performance-critical or not, which seems completely unfeasible.

[Drew Wilson]

On Wed, Apr 25, 2012 at 11:06 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Apr 25, 2012 at 11:02 AM, Andrew Wilson <atwi...@google.com> wrote:

BTW, can anyone actually think of a case where they would've written their code differently if a member function used CamelCase instead of linux_hacker style?

Well, the obvious one is that I always try to declare local variables to hold the results of CamelCase functions called more than once as long as the results should be constant, whereas with unix_hacker functions I often just call the function again.

Sure, and I'd argue that if this actually matters, we ought to code it that way to begin with, which would also allow the compiler to store the result in a register for even more optimal execution.

 

I don't know about larger architectural decisions.  I think I may have made a couple of those, but they seem much rarer.

I think some of my appreciation for unix_hacker functions comes from confusion when reading the views codebase, which originally used CamelCase for pretty much everything, and I found myself constantly wondering "why is THAT expensive?  Is it doing some sort of chained lookup on the... oh wait, I guess it's not really expensive, they were just lying to me."

Sure. Or perhaps they just didn't want to make "this function is trivial and will never take more time than a single memory read/write" a part of the API contract. In fact, as an API designer, I can't think of many times I'd want to make that claim.

Anyhow, I don't tend to be dogmatic about style issues, so I'm not really suggesting that we change this - just that we should recognize that there are real costs to this element of our style guide, and it's not clear there are commensurate benefits.

 

PK

[Peter Kasting]

On Wed, Apr 25, 2012 at 11:17 AM, Drew Wilson <atwi...@chromium.org> wrote:

Anyhow, I don't tend to be dogmatic about style issues, so I'm not really suggesting that we change this - just that we should recognize that there are real costs to this element of our style guide, and it's not clear there are commensurate benefits.

I agree with you, and I don't feel dogmatic about this rule.  I like it more than you do but I could be convinced to change it.

I think for me the most important point is really consistency.  The current codebase uses unix_hacker style for these cheap accessors much more than it uses CamelCase, and so I would only want to change the rule if we have a tool that can change over most of the code at the same time.  I would not want to see us change the rule for future code and then wind up with no consistency around naming.  This is why the views stuff was so confusing -- it clashed with the rest of the code.  If all our code had avoided unix_hacker naming I never would have been confused reading the views code.

PK