[doc] Struct members should have m

jnewbery commented at 9:15 am on August 18, 2020: member

For consistency, struct members should have the m_ prefix (in C++ structs are just classes with default public membership)

fanquake added the label Docs on Aug 18, 2020

[doc] Struct members should have m_ prefix 7b144f4a8c

in doc/developer-notes.md:84 in c53d595582 outdated

80@@ -81,7 +81,7 @@ required when doing so would need changes to significant pieces of existing
81 code.
82   - Variable (including function arguments) and namespace names are all lowercase and may use `_` to
83     separate words (snake_case).
84-    - Class member variables have a `m_` prefix.
85+    - Class and struct member variables have a `m_` prefix.

flack commented at 9:19 am on August 18, 2020:

0    - Class and struct member variables have an `m_` prefix.

jnewbery commented at 9:22 am on August 18, 2020:

thanks. Done.

jnewbery force-pushed on Aug 18, 2020

hebasto commented at 11:19 am on August 18, 2020: member

The m_ prefix is useful for the internal class code insofar it clear distinguishes a data member from local and global variables. I don’t thinks this is required for structs. And struct.m_data looks ugly for me.

Sorry.

ryanofsky commented at 11:26 am on August 18, 2020: member

I don’t like this change. It ignores the way struct and classes are used differently, and the reason m_ and g_ prefixes were introduced, which was to make it possible to distinguish local variable references from member variable and global variable references. For example with:

0var = 3;
1m_var = 3;
2g_var = 3;

the prefixes help you see without looking things up that var is a local variable, m_var is a member variable and g_var is a global variable, and helps you read and navigate code more quickly.

The m_ prefix doesn’t serve a purpose for structs because because implicit this isn’t used for struct accesses:

0s.var = 3;
1p->var = 4;

Clearly with s.var or p->var, var is a struct member. Replacing it with m_var just adds noise.

I think it makes sense to add code conventions when it solves problems, but it’s not clear what problem would be solved by adding this convention.

(Somewhat related: I wouldn’t mind having conventions that help make the differences between classes and structs clear. I’d be happy with a convention that encouraged class member variables not to be public, so you’d be unlikely to see c->m_var = 3, and a convention that would encourage structs not to have methods and only have public members, so the structs would be used properly. I think we basically do follow these conventions so there may be little need to write them down.)

jnewbery commented at 12:38 pm on August 18, 2020: member

I’m happy with either convention:

- Class and struct member variables have an 'm_' prefix.

or

- Class member variables have an 'm_' prefix (struct members do not have the 'm_' prefix since they're usually not accessed with implicit 'this').

but want it to be documented to avoid wasting time in review. Very happy to change it to the second version if that’s what people prefer.

hebasto commented at 12:41 pm on August 18, 2020: member

I’m happy with either convention:

- Class and struct member variables have an 'm_' prefix.

or

- Class member variables have an 'm_' prefix (struct members do not have the 'm_' prefix since they're usually not accessed with implicit 'this').

but want it to be documented to avoid wasting time in review. Very happy to change it to the second version if that’s what people prefer.

+1 for the second version.

jnewbery commented at 12:48 pm on August 18, 2020: member

I should also note that many people already seem to be following the convention of prefixing struct members with m_. See for example:

CNodeState and its sub-structs
CConnman::Options
CNodeStats (which is a class but should really be a struct since it only has public data members).

The newer data members in those structs are prefixed m_.

My vote is for the first option, since having a consistent rule across classes and structs is easier to state/follow, and people naturally seem to be doing it already.

promag commented at 10:50 pm on August 23, 2020: member

but want it to be documented to avoid wasting time in review. Very happy to change it to the second version if that’s what people prefer.

👍

I should also note that many people already seem to be following the convention of prefixing struct members with m_.

Because it’s in the developer notes?

FWIW I also prefer to skip m_ prefix in structs.

theStack commented at 6:24 pm on August 24, 2020: member

I think the division of opinions is partly caused by the fact that there is no clear definition of what a struct is in C++. Semantically it is clear, but is it really helpful to call a class with default public membership still a struct if you add member functions?

If there is no member functions (e.g. CNodeStats), I tend to agree with hebasto and ryanofsky that the m_ is pointless and doesn’t add value in readability. If there is at least one member function (e.g. CNodeState), you have the implicit this pointer and it can make sense.

I wouldn’t mind having conventions that help make the differences between classes and structs clear. I’d be happy with a convention that encouraged class member variables not to be public, so you’d be unlikely to see c->m_var = 3, and a convention that would encourage structs not to have methods and only have public members, so the structs would be used properly.

+1

ajtowns commented at 0:43 am on September 2, 2020: member

If there is no member functions (e.g. CNodeStats), I tend to agree with hebasto and ryanofsky that the m_ is pointless and doesn’t add value in readability. If there is at least one member function (e.g. CNodeState), you have the implicit this pointer and it can make sense.

+1 – maybe the guideline should be “if you have non-static member functions (EDIT: other than constructors), it should be a class and data members should be prefixed with m_; otherwise it should be a struct, and data members should be unprefixed. static data members should be prefixed with g_ as they are globals.” ?

EDIT: or perhaps “data members in classes should be prefixed with m_. structs should have no member functions other than constructors, no private/protected members, and any static data members should be constexpr”

sipa commented at 1:35 am on September 2, 2020: member

FWIW, when this rule was added, I intended it to apply to both structs and classes. There seem to be good reasons to not have it apply to member variables which are intended to be part of the public interface (which I guess would be a struct-like object), and it hasn’t been consistently applied, so I don’t think it’s a big setback to change the rule to not have the m_ prefix for such types.

I’m ok with changing it in either direction, but clarifying is good.

jnewbery commented at 8:11 am on September 24, 2020: member

I have a preference for applying to rules to both structs and classes for one reason only: simplicity. It’s a far simpler rule to consistently apply the rule “prefix all member variables with m_”, than “if you have non-static member functions other than constructors, it should be a class and data members should be prefixed with m_; otherwise it should be a struct, and data members should be unprefixed.”

I don’t think the distinction between class and struct is very important (as long as the private/public specifiers are used it doesn’t make a difference). I do think that having member variables prefixed with m_ is a big improvement for readability, so that should be the priority.

ryanofsky commented at 11:40 am on September 24, 2020: member

I don’t want to follow a strange rule that everybody agrees does not improve readability, and that I’ve never seen any other C or C++ project require, or even follow inadvertently.

m_ improves readability when implicit this is used, and not in other cases. There is a big difference between public data structures and classes with private state. Thousands of pages have been written about the wonders and magic of classes. C++ doesn’t enforce a difference because C++ barely enforces anything. But I don’t think you should falsely read into that and require different things to be the same. If you want to invent a new rule that people don’t like, that is new in 47 years of C programming, and that doesn’t provide any first order benefits, I think there should be a bigger second-order benefit than fake consistency in a poorly written document.

jnewbery commented at 12:22 pm on September 24, 2020: member

Ok, closing this. It seems to be antagonizing people unnecessarily. I don’t think adding guidance about when to use class or struct belongs in the Symbol naming conventions section, but if @ryanofsky wants to add it somewhere else in the developer docs, I’m happy to review.

If you want to invent a new rule that people don’t like, that is new in 47 years of C programming

It doesn’t matter too much, but I believe we copied this style convention from xbmc (https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#87-variables) which encourages using m_ for non-static member variables, and that does convention does appear to be used for structs in at least some places in that codebase (eg https://github.com/xbmc/xbmc/blob/28239ef93051ce4317beeea4cc0a79aa475841b7/xbmc/platform/win10/filesystem/WinLibraryFile.cpp#L50-L54).

jnewbery closed this on Sep 24, 2020

vasild commented at 1:28 pm on September 29, 2020: member

FWIW the Google C++ Style Guide makes a clear distinction when to use struct and when class and they have different naming rules for member variables.

jnewbery deleted the branch on Sep 29, 2020

DrahtBot locked this on Feb 15, 2022

[doc] Struct members should have m_ prefix #19759