doc: Tidy up shadowing section #16461

Removes the example because it violates the code format.

Not sure what is being "fixed" here

@MarcoFalke in the example the constructor argument wouldn't shadow the member.

I still don't get it … can you please keep changes contained to what you're actually fixing, and not change code structure at the same time

can you please keep changes contained to what you're actually fixing, and not change code structure at the same time

done

I still don't get it

the example is wrong considering the comment:

E.g. in member initializers, prepend _ to the argument name shadowing the member name:

so in order to be correct the argument must be the same as the member..

E.g. in member initializers, prepend _ to the argument name shadowing the member name:

This is wrong. We prepend members with m_ and pass arguments without m_. There should never be a shadow.

@MarcoFalke right, that's why initially I had

struct AddressBookPage
{
   Mode mode

Do you prefer to drop this case then?

Do you prefer to drop this case then?

Yes, if there is an incorrect statement in the notes, it should be removed.

This is wrong. We prepend members with m_ and pass arguments without m_. There should never be a shadow.

FWIW (feel free to ignore this comment), I don't think it makes sense to prefix struct members with m_. The point of m_ is to make member function code more readable, so readers will not confuse member variables with argument variables and local variables. In python, you wouldn't prefix members with m_ because you would say self. instead. In a struct, you shouldn't prefix members with m_ because there shouldn't be any non-trivial code using implicit this. Structs shouldn't any nontrivial member functions. But they could have simple helpers methods like SerializationOp, operator<, or convenience constructors where shadowing could happen, so it'd be reasonable to have convention for shadowing.

Anyway, I don't think we need to have a whole discussion about code style. I'm fine with any change you want to make. I'm just responding to the "this is wrong" claim above, which I don't think is factual, just expressing a different preference.

Yeah, I assumed struct is treated the same as class when it comes to the m_ prefix

I just want to fix the example which is obviously wrong in regards to shadowing.

I just want to fix the example which is obviously wrong in regards to shadowing.

Could drop the example and comment about shadowing since it should come up much more rarely now. Or use:

struct AddressBookPage {
    AddressBookPage(Mode _mode) : mode(_mode) {}
    Mode mode;
};

or

class AddressBookPage
{
public:
    AddressBookPage(Mode mode) : m_mode(mode) {}
private:
    Mode m_mode;
};

I think I always interpreted that section (and intended it) to mean structs and classes both get the m_ treatment. However, I do see that isn't very obvious from the text, and #16461 (comment) gives a pretty reasonable argument why it may be preferable not to apply it to structs.

So I guess we should clarify it, but I don't have a strong opinion either way.

I think we could have POD members without m_ prefix.

I think we could have POD members without m_ prefix.

Was about to say, that discussion is about POD versus OOP encapsulation, not about class versus struct per se—although there's definitely overlap between those distinctions how they're used normally

I think PODness is orthogonal here. Take a struct like InitInterfaces. It is definitely not a POD. But it has no methods and makes no use of implicit *this, so there is little reason to prefix its members with m_.

[EDIT] Other examples: WalletAddress, WalletTx:

https://github.com/bitcoin/bitcoin/blob/3f288a1c05ebcadd7d7709f81c77921ff9e27ba2/src/interfaces/wallet.h#L296-L308

https://github.com/bitcoin/bitcoin/blob/3f288a1c05ebcadd7d7709f81c77921ff9e27ba2/src/interfaces/wallet.h#L330-L344

Agree that whether something is integer or not should not influence the m_ style

Right, POD members must be POD too.

makes no use of implicit *this

Yup, should be enough to not require m_. The problem is that later on methods could be added which would screw existing member names.

I'll hold on with this until we clarify the m_ treatment, otherwise I'm tempted to either remove the example of just close this.

I think PODness is orthogonal here

OK, yes, POD is the not the right name either. It depends on whether the fields are supposed to be accessed externally or in methods.

otherwise I'm tempted to either remove the example of just close this.

I'd say let's just remove this example.

@promag Did you want to follow up here and remove the example?

Yes, will do.

@fanquake done.

unsigned ACK 9452802480bd154e23771230bbdfebde1dbaa941

ACK 9452802480bd154e23771230bbdfebde1dbaa941

ACK 9452802480bd154e23771230bbdfebde1dbaa941 - Thanks for following up.