Open Bug 1851469 Opened 1 year ago Updated 1 year ago

C++ coding style should specify whether to use free static functions or static methods

Categories

(support.mozilla.org :: Code Quality, defect)

defect

Tracking

(Not tracked)

People

(Reporter: mbrodesser-Igalia, Unassigned, NeedInfo)

Details

Example:

A.h:

class A {
  void m1();
  int z;
};

A.cpp:

// `m2(int x)` can be a static free function or a member of `A`.  

// `m3(int& z)` can be a static free function or a member of `A`, not needing to pass `z` as an argument but accessing it as a member instead.

// A.cpp
A::m1() {
  m2(5);
  // m3(z) or m3();
}

Creating clear rules for this simplifies writing patches, because reviewer's preferences differ.

Flags: needinfo?(bholley)

I'm not sure there's a one-size-fits-all answer here. Static member functions are more powerful because of their ability to access other private members on instances, and can also be useful as an organizing tactic. On the flip side, they require modifying the code in two places, so for really trivial things a static free function can be convenient.

My lean is that it's fine to leave this unspecified and lean on peoples' judgement. I'm open to incorporating the fuzzy guidance above into the style guide, but it would require some effort to make sure we get all the nuance right and socialize it appropriately, and absent evidence that this is coming up frequently I'm not convinced it's worth it.

Flags: needinfo?(bholley)

(In reply to Bobby Holley (:bholley) from comment #2)

I'm not sure there's a one-size-fits-all answer here. Static member functions are more powerful because of their ability to access other private members on instances, and can also be useful as an organizing tactic.

Can you please explain the latter?

On the flip side, they require modifying the code in two places, so for really trivial things a static free function can be convenient.

My lean is that it's fine to leave this unspecified and lean on peoples' judgement. I'm open to incorporating the fuzzy guidance above into the style guide, but it would require some effort to make sure we get all the nuance right and socialize it appropriately, and absent evidence that this is coming up frequently I'm not convinced it's worth it.

A rule for the simple case exemplified above, m2(int x) would be helpful. Be also aware of the relation to unified builds.

Olli: can you please share your gut feeling how often this topic comes up in reviews?

Flags: needinfo?(smaug)
Flags: needinfo?(bholley)

I agree with bholley here. static member functions often help with organizing code (like in the case I requested them) , but they aren't always needed. It depends a bit on the case. FWIW, Google C++ coding style is also rather vague on this.
But at least one shouldn't have other static functions which then in the naming hint that they are supposed to be used by a certain class only.
In those cases the static method should be part of the class.

Flags: needinfo?(smaug)

Thanks, Olli. Mind sharing your gut feeling about how often this comes up in reviews?

Flags: needinfo?(smaug)

How would I say this politely :) The style you proposed in some earlier patches felt very unusual.
Nothing wrong with that style, but just not very common in the code I've looked at.

There are cases when some simple local helper static method is added to .cpp, but not in a way where such methods basically implement the main part of a class method.

Flags: needinfo?(smaug)
You need to log in before you can comment on or make changes to this bug.