Re: style convention: /*foo_p=*/ to annotate bool arguments

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

On 4 October 2016 at 10:21, David Brown wrote:
> On 04/10/16 01:48, Martin Sebor wrote:
>> In a recent review Jason and I discussed the style convention
>> commonly followed in the C++ front end to annotate arguments
>> in calls to functions taking bool parameters with a comment
>> along the lines of
>>
>>   foo (1, 2, /*bar_p=*/true);

I like this convention at the call-site, otherwise "true" or "false"
doesn't tell you much and you have to look at the declaration.

IMHO even better is to not use bool and define an enumeration type, so
the call site has something unambiguous like foo (1, 2, yes_bar) or
foo (1, 2, no_bar).


>> I pointed out to Jason that in another code review, Jeff asked
>> to remove the same comment from the middle-end [1].  In the
>> interest of consistency Jason and I thought I should bring this
>> up for discussion so we can all be clear on whether or not this
>> is something worth standardizing and documenting.
>>
>> As a separate question, in the same discussion I mention to Jason
>> a convention that I myself have found useful where the value of
>> a default argument is indicated in a comment in a function
>> definition that is declared elsewhere to take one, e.g., like so:
>>
>>   // In some header:
>>   void foo (int, int, bool = -1);
>>
>>   // In some .c file:
>>   void foo (int x, int y, bool bar_p /* = false */)
>>   {
>>     ...
>>   }
>>
>> Jason thought this would be a good convention.  Is there any
>> interest in/support for adopting it?
>>
>
> IMHO, a commenting convention that makes it easy to write incorrect and
> misleading comments is far worse than a convention that omits the
> comments entirely.
>
> I am not involved in gcc development (I'm just an interested observer on
> this list), so I don't know much about the coding style in gcc at the
> moment.  But /I/ would prefer a style like this:
>
> // In some header:
> void foo(int x, int y, bool bar_p = true);
>
> // In some .c file:
> void foo(int x, int y, bool bar_p)
> {
> ...
> }
>
> There are several differences, which gcc folks may or may not agree
> with, but which might be worth considering:
>
> 1. "bool" should be either "true" or "false" - not an integer.

I assume the -1 default argument was a typo and that nobody is
proposing using -1 as a sensible bool value. I hope that's the case
anyway :-)


> 2. I think parameter names are important in declarations - they help
> tell you what the function is, and how to use it.

Of course.

> (As an aside, I would /really/ like a gcc warning option that would
> check for consistency in parameter names.  This would be particularly
> useful if C++ ever gets a named parameter feature.)
>
> 3. In the implementation, there is /no/ comment about the default value
> of "bar_p".  The implementation has to work correctly regardless of the
> value of "bar_p", so knowing the default value is of little benefit.

In general I agree, although it could be useful in a more realistic
example. If the parameter with a default argument is a 'tree' and
could be null, then the comment serves as a reminder that callers
might not have passed a valid tree:

int frob_trees(tree lhs, tree rhs /* = NULL_TREE */)
{
  // Handle the case where only one argument is passed:
  if (rhs == NULL_TREE)
    rhs = lhs;  // or something more sensible.
  // ...
}

Even here it's not hugely valuable, as the "Handle the case ..."
comment can convey the same information.


> If
> it is relevant, perhaps because you want to emphasize a particular path
> for speed, then a comment might be useful - but it is better to use code
> (like __builtin_expect) than comments.  Comments make it easy to be
> inconsistent, and are unnecessary when the same information is easily
> available by looking at the header (modern IDE's make that quick).

That's not very relevant if GCC developers don't use modern IDEs :-)