Coding standards proposal, usage of "this"

Coding standards proposal, usage of "this"

[Simon Marchi]

Hi all,

Here's something I had in mind for a while.  We don't consistently use
`this` when referring to fields or methods of the current object.  I
never now if I should use it or not, or point it out in review.  I
therefore propose these rules so that we have something to refer to.

 - Use `this` when referring to a data member that is not prefixed by
   `m_`.  Rationale: without `this`, it's not clear that you are
   referring to a member of the current class, versus a local or global
   variable.
 - Don't use `this` when referring to a data member that is prefixed by
   `m_`.  Rationale: the prefix already makes it clear that you are
   referring to a member of the current class, so adding `this` would
   just add noise.
 - Use `this` when referring to a method of the current class.
   Rationale: without `this, it's not clear that you are referring to a
   method of the current class, versus a free function.

If we had a convention for how to name internal helper methods, which
would make it clear that they are methods of the current object and not
free functions (a bit like `m_` does for data members), we could omit
`this` when calling such a method.  But we don't have that at the
moment.

A concrete example:

  int a_global;

  struct a_struct
  {
    int a_method ()
    {
      int a_local = 17;

      return (a_local
	      + a_global
	      + m_a_private_field
	      + this->a_public_field
	      + this->a_helper_method ());
    }

    int a_public_field;

  private:
    int a_helper_method ()
    {
      return m_a_private_field;
    }

    int m_a_private_field;
  };

Any comments?  My intention would be to add this to the coding standards
on the wiki.

Simon

Re: Coding standards proposal, usage of "this"

[Paul Koning]

> On Aug 13, 2021, at 10:26 AM, Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> Hi all,
> 
> Here's something I had in mind for a while.  We don't consistently use
> `this` when referring to fields or methods of the current object.  I
> never now if I should use it or not, or point it out in review.  I
> therefore propose these rules so that we have something to refer to.
> 
> - Use `this` when referring to a data member that is not prefixed by
>   `m_`.  Rationale: without `this`, it's not clear that you are
>   referring to a member of the current class, versus a local or global
>   variable.
> - Don't use `this` when referring to a data member that is prefixed by
>   `m_`.  Rationale: the prefix already makes it clear that you are
>   referring to a member of the current class, so adding `this` would
>   just add noise.
> - Use `this` when referring to a method of the current class.
>   Rationale: without `this, it's not clear that you are referring to a
>   method of the current class, versus a free function.
> 
> ...
> 
> Any comments?  My intention would be to add this to the coding standards
> on the wiki.

This roughly amounts to turning C++ coding conventions into those of Python.  While I like the Python ones, it seems odd to force C++ style into something clearly not envisioned by its basic structure.

The fact that C++ requires "this" in a few obscure cases is due to messy add-ons like templates.  I don't see that as a reason to put "this" everywhere else.  

	paul

Re: Coding standards proposal, usage of "this"

[Simon Marchi]

On 2021-08-13 10:46 a.m., Paul Koning wrote:
> The fact that C++ requires "this" in a few obscure cases is due to messy add-ons like templates.  I don't see that as a reason to put "this" everywhere else.  

The reason does not have anything to do with templates, it's to help readability.

Simon

Re: Coding standards proposal, usage of "this"

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-08-13 10:26:48 -0400]:

> Hi all,
> 
> Here's something I had in mind for a while.  We don't consistently use
> `this` when referring to fields or methods of the current object.  I
> never now if I should use it or not, or point it out in review.  I
> therefore propose these rules so that we have something to refer to.
> 
>  - Use `this` when referring to a data member that is not prefixed by
>    `m_`.  Rationale: without `this`, it's not clear that you are
>    referring to a member of the current class, versus a local or global
>    variable.
>  - Don't use `this` when referring to a data member that is prefixed by
>    `m_`.  Rationale: the prefix already makes it clear that you are
>    referring to a member of the current class, so adding `this` would
>    just add noise.
>  - Use `this` when referring to a method of the current class.
>    Rationale: without `this, it's not clear that you are referring to a
>    method of the current class, versus a free function.
> 
> If we had a convention for how to name internal helper methods, which
> would make it clear that they are methods of the current object and not
> free functions (a bit like `m_` does for data members), we could omit
> `this` when calling such a method.  But we don't have that at the
> moment.

I'd be OK with extending the use of the m_ prefix for private member
functions.  But that wouldn't change the above rules, which all make
sense to me.

Thanks,
Andrew


> 
> A concrete example:
> 
>   int a_global;
> 
>   struct a_struct
>   {
>     int a_method ()
>     {
>       int a_local = 17;
> 
>       return (a_local
> 	      + a_global
> 	      + m_a_private_field
> 	      + this->a_public_field
> 	      + this->a_helper_method ());
>     }
> 
>     int a_public_field;
> 
>   private:
>     int a_helper_method ()
>     {
>       return m_a_private_field;
>     }
> 
>     int m_a_private_field;
>   };
> 
> Any comments?  My intention would be to add this to the coding standards
> on the wiki.
> 
> Simon

Re: Coding standards proposal, usage of "this"

[Lancelot SIX]

On Fri, Aug 13, 2021 at 10:26:48AM -0400, Simon Marchi via Gdb-patches wrote:
> Hi all,
> 
> Here's something I had in mind for a while.  We don't consistently use
> `this` when referring to fields or methods of the current object.  I
> never now if I should use it or not, or point it out in review.  I
> therefore propose these rules so that we have something to refer to.
> 
>  - Use `this` when referring to a data member that is not prefixed by
>    `m_`.  Rationale: without `this`, it's not clear that you are
>    referring to a member of the current class, versus a local or global
>    variable.
>  - Don't use `this` when referring to a data member that is prefixed by
>    `m_`.  Rationale: the prefix already makes it clear that you are
>    referring to a member of the current class, so adding `this` would
>    just add noise.
>  - Use `this` when referring to a method of the current class.
>    Rationale: without `this, it's not clear that you are referring to a
>    method of the current class, versus a free function.
> 
> If we had a convention for how to name internal helper methods, which
> would make it clear that they are methods of the current object and not
> free functions (a bit like `m_` does for data members), we could omit
> `this` when calling such a method.  But we don't have that at the
> moment.
> 

Hi,

I think I tend to use both ('this->' and 'm_' prefix) because the 'm_'
prefix is used and I have some python habit of using 'self'.

I am absolutely fine with your approach to use exactly one of those two
markers since both says that they are referring to a member of the
class, and having both is redundant.  Also, having a rule is a good
thing, so count me in.

Extending the same sort of convention to helper methods is also
reasonable.  I do not have an opinion on what should be the prefix.
However, classes and structs will more likely have both public (without
prefix) and protected/private (with prefix) methods, which can lead to a
somewhat less uniform style when writing methods body.  I tend to think
that for this reason just using 'this->' all the time might be easier,
but this is not a strong opinion, I am fine using a prefix.  On the
other hand, having a prefix can be a nice hint when refactoring code
that a given method is not part of the public API and therefore easier
to change.

Lancelot.

Re: Coding standards proposal, usage of "this"

[Christian Biesinger]

On Fri, Aug 13, 2021 at 10:27 AM Simon Marchi via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> Hi all,
>
> Here's something I had in mind for a while.  We don't consistently use
> `this` when referring to fields or methods of the current object.  I
> never now if I should use it or not, or point it out in review.  I
> therefore propose these rules so that we have something to refer to.
>
>  - Use `this` when referring to a data member that is not prefixed by
>    `m_`.  Rationale: without `this`, it's not clear that you are
>    referring to a member of the current class, versus a local or global
>    variable.
>  - Don't use `this` when referring to a data member that is prefixed by
>    `m_`.  Rationale: the prefix already makes it clear that you are
>    referring to a member of the current class, so adding `this` would
>    just add noise.

Those two seem good.

>  - Use `this` when referring to a method of the current class.
>    Rationale: without `this, it's not clear that you are referring to a
>    method of the current class, versus a free function.

I'm not really a fan of this, and I am also not aware of other
projects using such a style. Since this makes calling member functions
more verbose/uglier than calling free functions, this would also
discourage member functions. Is there a need to distinguish these?

Christian

Re: Coding standards proposal, usage of "this"

[Simon Marchi]

>>  - Use `this` when referring to a method of the current class.
>>    Rationale: without `this, it's not clear that you are referring to a
>>    method of the current class, versus a free function.
> 
> I'm not really a fan of this, and I am also not aware of other
> projects using such a style. Since this makes calling member functions
> more verbose/uglier than calling free functions, this would also
> discourage member functions. Is there a need to distinguish these?

There's no *need*, of course.  It's just based on my experience, I
remember seeing some function calls, wondering how it could even work
with the passed arguments.  And then I realized it was a method call,
where `this` is passed implicitly.

Do you think it would discourage adding member functions, in opposition
to adding free functions?  Or just leaving the code in-line, leading to
bigger methods?

Simon

Re: Coding standards proposal, usage of "this"

[Ruslan Kabatsayev]

On Mon, 16 Aug 2021 at 20:01, Simon Marchi via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> >>  - Use `this` when referring to a method of the current class.
> >>    Rationale: without `this, it's not clear that you are referring to a
> >>    method of the current class, versus a free function.
> >
> > I'm not really a fan of this, and I am also not aware of other
> > projects using such a style. Since this makes calling member functions
> > more verbose/uglier than calling free functions, this would also
> > discourage member functions. Is there a need to distinguish these?
>
> There's no *need*, of course.  It's just based on my experience, I
> remember seeing some function calls, wondering how it could even work
> with the passed arguments.  And then I realized it was a method call,
> where `this` is passed implicitly.
>
> Do you think it would discourage adding member functions, in opposition
> to adding free functions?  Or just leaving the code in-line, leading to
> bigger methods?

This will prevent the "prefer nonmember, nonfriends" mantra from
working. Suppose you made a free function (so as to prevent accidental
coupling to the private state of your class), and later it happens to
need access to the state. This function is then turned into a
(possibly const) method. And now... you have to alter all the callers
to prefix the calls with `this->`, even though it's syntactically
useless. Moreover, it may be easy to miss some, so unless there's a
static analyzer checking for this rule, this leads to inconsistency.

>
> Simon

Re: Coding standards proposal, usage of "this"

[John Baldwin]

On 8/13/21 7:26 AM, Simon Marchi via Gdb-patches wrote:
> Hi all,
> 
> Here's something I had in mind for a while.  We don't consistently use
> `this` when referring to fields or methods of the current object.  I
> never now if I should use it or not, or point it out in review.  I
> therefore propose these rules so that we have something to refer to.
> 
>   - Use `this` when referring to a data member that is not prefixed by
>     `m_`.  Rationale: without `this`, it's not clear that you are
>     referring to a member of the current class, versus a local or global
>     variable.
>   - Don't use `this` when referring to a data member that is prefixed by
>     `m_`.  Rationale: the prefix already makes it clear that you are
>     referring to a member of the current class, so adding `this` would
>     just add noise.

These seem fine to me.

>   - Use `this` when referring to a method of the current class.
>     Rationale: without `this, it's not clear that you are referring to a
>     method of the current class, versus a free function.

This one feels a bit odd to me, though it may just be something I'm not
used to.  It is something I haven't seen used before in C++ at least.

-- 
John Baldwin

Re: Coding standards proposal, usage of "this"

[Simon Marchi]

On 2021-08-16 1:06 p.m., John Baldwin wrote:
> On 8/13/21 7:26 AM, Simon Marchi via Gdb-patches wrote:
>> Hi all,
>>
>> Here's something I had in mind for a while.  We don't consistently use
>> `this` when referring to fields or methods of the current object.  I
>> never now if I should use it or not, or point it out in review.  I
>> therefore propose these rules so that we have something to refer to.
>>
>>   - Use `this` when referring to a data member that is not prefixed by
>>     `m_`.  Rationale: without `this`, it's not clear that you are
>>     referring to a member of the current class, versus a local or global
>>     variable.
>>   - Don't use `this` when referring to a data member that is prefixed by
>>     `m_`.  Rationale: the prefix already makes it clear that you are
>>     referring to a member of the current class, so adding `this` would
>>     just add noise.
> 
> These seem fine to me.
> 
>>   - Use `this` when referring to a method of the current class.
>>     Rationale: without `this, it's not clear that you are referring to a
>>     method of the current class, versus a free function.
> 
> This one feels a bit odd to me, though it may just be something I'm not
> used to.  It is something I haven't seen used before in C++ at least.

So, the first two seem to be more accepted, and this last one less.  I'd
be fine just going with the first two then (even though in my opinion
the reason for using `this` to refer to a non-prefixed data member
applies the same when referring to a non-prefied member function).

Simon

Re: Coding standards proposal, usage of "this"

[Luis Machado]

On 8/16/21 2:11 PM, Simon Marchi via Gdb-patches wrote:
> On 2021-08-16 1:06 p.m., John Baldwin wrote:
>> On 8/13/21 7:26 AM, Simon Marchi via Gdb-patches wrote:
>>> Hi all,
>>>
>>> Here's something I had in mind for a while.  We don't consistently use
>>> `this` when referring to fields or methods of the current object.  I
>>> never now if I should use it or not, or point it out in review.  I
>>> therefore propose these rules so that we have something to refer to.
>>>
>>>    - Use `this` when referring to a data member that is not prefixed by
>>>      `m_`.  Rationale: without `this`, it's not clear that you are
>>>      referring to a member of the current class, versus a local or global
>>>      variable.
>>>    - Don't use `this` when referring to a data member that is prefixed by
>>>      `m_`.  Rationale: the prefix already makes it clear that you are
>>>      referring to a member of the current class, so adding `this` would
>>>      just add noise.
>>
>> These seem fine to me.
>>
>>>    - Use `this` when referring to a method of the current class.
>>>      Rationale: without `this, it's not clear that you are referring to a
>>>      method of the current class, versus a free function.
>>
>> This one feels a bit odd to me, though it may just be something I'm not
>> used to.  It is something I haven't seen used before in C++ at least.
> 
> So, the first two seem to be more accepted, and this last one less.  I'd
> be fine just going with the first two then (even though in my opinion
> the reason for using `this` to refer to a non-prefixed data member
> applies the same when referring to a non-prefied member function).

My 2 cents. I wouldn't mind the change, but having to remember when to 
use "this" and when not to use it is worse to me than spending a couple 
minutes trying to figure out why the code is the way it is.

I suppose it is just the nature of C++. Some constructs are just not 
great when trying to read/parse them. I think the same happens with some 
templates and lambda's. It might take a little bit to figure out where 
the functions are defined.

Given GDB's code base is a mix of C and C++, wouldn't we risk having yet 
another mix of new coding standards with old coding standards?

Re: Coding standards proposal, usage of "this"

[Simon Marchi]

> My 2 cents. I wouldn't mind the change, but having to remember when to use "this" and when not to use it is worse to me than spending a couple minutes trying to figure out why the code is the way it is.

Hmm, I think that when the rule is logical and you know the rationale,
it's pretty obvious when to use it or not.  In my original proposal:

 - does the identifier name contains something that makes it obvious
   it's a member (the m_ prefix)?  No need for `this`.
 - otherwise? Use `this`.

> I suppose it is just the nature of C++. Some constructs are just not great when trying to read/parse them. I think the same happens with some templates and lambda's. It might take a little bit to figure out where the functions are defined.
>
> Given GDB's code base is a mix of C and C++, wouldn't we risk having yet another mix of new coding standards with old coding standards?

Yes, but in my opinion we shouldn't refrain of adopting new rules and
standards that we find useful because the existing code doesn't follow
the new rule.  Otherwise we could never introduce any new rule /
guideline / standards.

Simon

Re: Coding standards proposal, usage of "this"

[Andrew Burgess]

* Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2021-08-16 14:23:42 -0300]:

> On 8/16/21 2:11 PM, Simon Marchi via Gdb-patches wrote:
> > On 2021-08-16 1:06 p.m., John Baldwin wrote:
> > > On 8/13/21 7:26 AM, Simon Marchi via Gdb-patches wrote:
> > > > Hi all,
> > > > 
> > > > Here's something I had in mind for a while.  We don't consistently use
> > > > `this` when referring to fields or methods of the current object.  I
> > > > never now if I should use it or not, or point it out in review.  I
> > > > therefore propose these rules so that we have something to refer to.
> > > > 
> > > >    - Use `this` when referring to a data member that is not prefixed by
> > > >      `m_`.  Rationale: without `this`, it's not clear that you are
> > > >      referring to a member of the current class, versus a local or global
> > > >      variable.
> > > >    - Don't use `this` when referring to a data member that is prefixed by
> > > >      `m_`.  Rationale: the prefix already makes it clear that you are
> > > >      referring to a member of the current class, so adding `this` would
> > > >      just add noise.
> > > 
> > > These seem fine to me.
> > > 
> > > >    - Use `this` when referring to a method of the current class.
> > > >      Rationale: without `this, it's not clear that you are referring to a
> > > >      method of the current class, versus a free function.
> > > 
> > > This one feels a bit odd to me, though it may just be something I'm not
> > > used to.  It is something I haven't seen used before in C++ at least.
> > 
> > So, the first two seem to be more accepted, and this last one less.  I'd
> > be fine just going with the first two then (even though in my opinion
> > the reason for using `this` to refer to a non-prefixed data member
> > applies the same when referring to a non-prefied member function).
> 
> My 2 cents. I wouldn't mind the change, but having to remember when to use
> "this" and when not to use it is worse to me than spending a couple minutes
> trying to figure out why the code is the way it is.

Except most code is written once, and debugged many times.  So in the
long run it's much more efficient to spend a minute longer writing
the code, even if it saves just a few seconds off reading (and
understanding) the code.

IMHO, of course :)

Thanks,
Andrew

Re: Coding standards proposal, usage of "this"

[John Baldwin]

On 8/16/21 10:11 AM, Simon Marchi wrote:
> On 2021-08-16 1:06 p.m., John Baldwin wrote:
>> On 8/13/21 7:26 AM, Simon Marchi via Gdb-patches wrote:
>>> Hi all,
>>>
>>> Here's something I had in mind for a while.  We don't consistently use
>>> `this` when referring to fields or methods of the current object.  I
>>> never now if I should use it or not, or point it out in review.  I
>>> therefore propose these rules so that we have something to refer to.
>>>
>>>    - Use `this` when referring to a data member that is not prefixed by
>>>      `m_`.  Rationale: without `this`, it's not clear that you are
>>>      referring to a member of the current class, versus a local or global
>>>      variable.
>>>    - Don't use `this` when referring to a data member that is prefixed by
>>>      `m_`.  Rationale: the prefix already makes it clear that you are
>>>      referring to a member of the current class, so adding `this` would
>>>      just add noise.
>>
>> These seem fine to me.
>>
>>>    - Use `this` when referring to a method of the current class.
>>>      Rationale: without `this, it's not clear that you are referring to a
>>>      method of the current class, versus a free function.
>>
>> This one feels a bit odd to me, though it may just be something I'm not
>> used to.  It is something I haven't seen used before in C++ at least.
> 
> So, the first two seem to be more accepted, and this last one less.  I'd
> be fine just going with the first two then (even though in my opinion
> the reason for using `this` to refer to a non-prefixed data member
> applies the same when referring to a non-prefied member function).

I would say I'm more on the fence about the third option.  One point I
almost brought up that you mentioned in your reply to Christian is whether
or not this would encourage the use of more static free functions rather
than private member methods.  This is somewhat interesting since GDB still
has a lot of "C" code.  For example, in my async FreeBSD series I added
a private "wait_1" method to fbsd_nat_target in fbsd-nat.c that is
functionally identical to 'linux_nat_wait_1' in linux-nat.c (which is
not a member method, but a static free function).  My initial thought was
that this might discourage adding more private members, but OTOH, right
now linux_nat_target::wait() uses 'linux_nat_wait_1' to invoke the helper
while my modified fbsd_nat_target::wait() uses just 'wait_1'.  Changing
it to 'this->wait_1' instead would still be shorter than what is done
in linux-nat.c.  On the question of static free functions vs private
methods, I do think private methods is probably the "right" thing to do
for new changes going forward as using C++'s namespacing is cleaner than
what was previously done manually in C.  I'm not really sure adding
'this' prefixes would discourage this.  If you want a case in point to
evaluate, perhaps look at the last patch in my FreeBSD async series and
the various 'async_file_*' methods I added in inf_ptrace_target for use
in subclasses.

-- 
John Baldwin