Re: Coding standards proposal, usage of "this"

[John Baldwin <jhb@FreeBSD.org>]

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