Proposal: Remove 'Leave obvious parameter names out of function declarations' from Blink C++ style

[TAMURA, Kent]

I propose to remove the following rule in Blink C++ style guide.

Leave obvious parameter names out of function declarations

I'll remove the rule If no one objects to this proposal in a week.

Reason:

  * This rule isn't compatible with Google C++ style.

  Such parameter names are allowed in Google C++ style, and not allowed in Blink.

  * Need workarounds to mention such parameters in a comment

  We need to write   

    // The AttributeModificationParamas argument is blah blah...

    void ParseAttribute(const AttributeModificationParams&);

  or

    // |params| is blah blah...

    void ParseAttribute(const AttributeModificationParams& /* params */);

   instead of

    // |params| is blah blah...

    void ParseAttribute(const AttributeModificationParams& params);

  * It prevents us from making C++ code match to IDL.

  For example, Node IDL has:

    Node insertBefore(Node node, Node? child);

  However, our Node.h should not be:

    Node* insertBefore(Node* node, Node* child);

  due to this rule. We need to change the first argument to a different name or need to remove it.

  * Benefit of this rule is small.  It just makes the code shorter.

AFAIK, Google C++ style doesn't assume parameter names mandatory.  So we can continue to omit parameter names even if we remove the rule.

--

TAMURA Kent
Software Engineer, Google

[Daniel Cheng]

Sounds good to me.

While I did kind of like the fact that we could easily omit parameter names for long types in headers, having more consistency with Google C++ style and making it easier to comment on these parameters seem like good justifications to me.

Daniel

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAGH7WqHr2_HU%3DprvjUZmwnZNL2CZDq2OpWymxCmzDbhits18Fg%40mail.gmail.com.

[Victor Costan]

Making sure I understand correctly -- this means that omitting parameter names when they're obvious is optional, not that it's forbidden. Is this correct?

My interpretation is based on "Unused parameters that are obvious from context may be omitted" in https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions but the examples there are only super-obvious cases. Our old presubmit rule also triggered when param names were a substring of the type name (if I remember correctly) -- is it correct to assume that I can still omit parameter names in that case?

Thank you,

    Victor

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAF3XrKrd%2BQiVD8Buxf_hmotcpE1hAbH5Tf7JM3o0BFamrsrtFw%40mail.gmail.com.

[Peter Kasting]

On Wed, Mar 14, 2018 at 11:13 PM Victor Costan <pwn...@chromium.org> wrote:

Making sure I understand correctly -- this means that omitting parameter names when they're obvious is optional, not that it's forbidden. Is this correct?

That is indeed what the root post here proposes, and what the Google style guide says.

PK

[Daniel Cheng]

Ah yes, I forgot that the Google style guide changed to allow omission of obvious names. Dropping the rule to require omission of obvious parameter names definitely sounds like the right thing to do then.

Daniel

[Peter Kasting]

On Wed, Mar 14, 2018 at 11:58 PM Daniel Cheng <dch...@chromium.org> wrote:

Ah yes, I forgot that the Google style guide changed to allow omission of obvious names.

Hmm, actually you made me go double-check this.  The style guide says two things (in http://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions ):

"A parameter name may be omitted only if the parameter is not used in the function's definition"

"Unused parameters that are obvious from context may be omitted"

Given these statements and their surrounding context ("declarations and definitions") and examples , I read the rules as:

* Declarations and definitions should be in sync: don't omit the name in the declaration but preserve it in the definition.

* Since you can't omit the name in the definition unless it's unused, that means you can't omit it in the declaration unless it's unused in the definition.

* And even if it's unused in the definition, still don't omit it in the declaration if it's not obvious from context.

So in short, the Google style guide allows omission of parameter names, but in more limited scenarios than Blink used to require their omission.

In either case removing the Blink wording that contradicts this seems like a beneficial thing overall, and the result will be that Blink is governed by the Google style guide; it's just a matter of what the style guide allows.

PK

[Victor Costan]

To clarify: I'm not debating removing the Blink rule. I don't have great memory, so fewer rules help me be a more consistent (and thus better) reviewer.

I'm asking for guidance in understanding what this means for me as a code reviewer and author. The Google C++ style guide seems to be mildly confusing, as you pointed out.

If I understand correctly, your interpretation is that:

1) Parameter names in a declaration may only be omitted if they're also omitted in the definition (so, they must be unused).

2) Even if a parameter is unused, it may not be omitted if its name is not clear from the context.

This is significantly stricter than what I understood, but I agree that the text supports your interpretation better than mine. This makes me think a PSA to blink-dev might help.

Do you think it'd be useful to ask for a clarification in the Google C++ styleguide? If so, I can try to do that and come back with answers (and potentially pushed edits).

Thank you,

    Victor

[Peter Kasting]

On Thu, Mar 15, 2018 at 12:30 AM Victor Costan <pwn...@chromium.org> wrote:

If I understand correctly, your interpretation is that:

1) Parameter names in a declaration may only be omitted if they're also omitted in the definition (so, they must be unused).

2) Even if a parameter is unused, it may not be omitted if its name is not clear from the context.

Yes.

This is significantly stricter than what I understood, but I agree that the text supports your interpretation better than mine. This makes me think a PSA to blink-dev might help.

Do you think it'd be useful to ask for a clarification in the Google C++ styleguide? If so, I can try to do that and come back with answers (and potentially pushed edits).

I will never be opposed to people asking for style guide clarifications.  Either they'll say I'm wrong, which would be good to know, or that I'm right, which will be good to confirm, or they won't answer, in which case we haven't lost anything.

PK 

[TAMURA, Kent]

It seems I overlooked the statements in the style guide.  Thank you for pointing them out.

So, I was wrong. We need to add parameter names in many cases if we remove the Blink style rule.

I don't have a plan to implement omission rule of Google style in check-webkit-style, and existing code won't produce new warnings. I agree that we need an announcement to tell the new rule.

[Peter Kasting]

On Thu, Mar 15, 2018 at 3:15 AM TAMURA, Kent <tk...@chromium.org> wrote:

So, I was wrong. We need to add parameter names in many cases if we remove the Blink style rule.

Well, that's if you want to convert your existing code to be compliant.  In the past, we've often implemented style updates by saying "do it this way going forward" but not trying to retroactively bring code into compliance.  I think that's reasonable here.

PK

[Jeremy Roman]

I don't have strong feelings here.

I don't think I've seen the comment issue much, but consistency with Chromium/Google style in general is definitely a plus.

(I personally like this rule, though; the Google rule of "may omit if it is unused in the definition" has never struck me as being particularly useful, and it remains unclear to me how it applies to =default members, and the shown example seems to contradict the wording, as the Foo constructors presumably do use the argument in their definition.)

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAAHOzFDBw4sQ66p5QWb3OgVbOPEROkfJz%3DFKXENXFePMHSHcoA%40mail.gmail.com.

[Peter Kasting]

On Thu, Mar 15, 2018 at 10:15 AM Jeremy Roman <jbr...@chromium.org> wrote:

(I personally like this rule, though; the Google rule of "may omit if it is unused in the definition" has never struck me as being particularly useful, and it remains unclear to me how it applies to =default members, and the shown example seems to contradict the wording, as the Foo constructors presumably do use the argument in their definition.)

I figure that example is that way because the definition is likely "= default", which also answers the "= default" question :)

(Personally, I like never omitting param names, and if people really want to omit them the narrow Google rule seems like a good one to me because it keeps declaration and definition looking alike, which I value.  But I also don't care much.)

PK

[Emil A Eklund]

So somehow in this discussion we seem to have gone from "not requiring
obvious parameters to be omitted" to "not allowing parameters to be
omitted". I'm perfectly fine with dropping the requirement to omit but
I do object to not allowing it. We have *a lot* of very specific types
used as parameters in blink and requiring all of them to be named in
the definition will lead to very long lines and multi-line function
definitions. Hurting readability without adding any value.

[Peter Kasting]

On Thu, Mar 15, 2018 at 10:47 AM Emil A Eklund <e...@chromium.org> wrote:

So somehow in this discussion we seem to have gone from "not requiring
obvious parameters to be omitted" to "not allowing parameters to be
omitted".

To make sure there's no miscommunication, no one has proposed banning parameter omission -- the Google style guide allows it, it just allows it in fewer cases than Blink currently requires it.

The closest we got was me saying I personally like when names are never omitted, but I'm not proposing that govern the project.

I'm perfectly fine with dropping the requirement to omit but
I do object to not allowing it. We have *a lot* of very specific types
used as parameters in blink and requiring all of them to be named in
the definition will lead to very long lines and multi-line function
definitions. Hurting readability without adding any value.

Can you describe how Blink's needs differ from google3's here that would suggest we should keep some kind of Blink-specific rule?

PK

[Emil A Eklund]

On Thu, Mar 15, 2018 at 11:22 AM, Peter Kasting <pkas...@chromium.org> wrote:
> On Thu, Mar 15, 2018 at 10:47 AM Emil A Eklund <e...@chromium.org> wrote:
>>
>> So somehow in this discussion we seem to have gone from "not requiring
>> obvious parameters to be omitted" to "not allowing parameters to be
>> omitted".
>
> To make sure there's no miscommunication, no one has proposed banning
> parameter omission -- the Google style guide allows it, it just allows it in
> fewer cases than Blink currently requires it.

While technically true what you are proposing is changing from
*requiring* omission to *disallowing* omission in the vast majority of
cases. The one allowed case for omission being obvious and *unused*
parameters which is much more restrictive and doesn't really come into
play in blink much at all.

That is a pretty big style guide change.

> The closest we got was me saying I personally like when names are never
> omitted, but I'm not proposing that govern the project.
>
>> I'm perfectly fine with dropping the requirement to omit but
>> I do object to not allowing it. We have *a lot* of very specific types
>> used as parameters in blink and requiring all of them to be named in
>> the definition will lead to very long lines and multi-line function
>> definitions. Hurting readability without adding any value.
>
> Can you describe how Blink's needs differ from google3's here that would
> suggest we should keep some kind of Blink-specific rule?

The Blink code base for the longest time strongly discouraged comments
and instead encouraged very specific type and function names. As such
we have type names that are super specific and quite long, negating
the need for parameter names as they add no value.
I'd also argue that the intent behind the recent google3 style change
to allow omission is to allow this type of omission however it seems
we interpret that rule differently.

Thanks for listening.

[Harald Alvestrand]

I regularly get dinged by check-webkit-style because I've written functions the same way in Blink as in content/. So I definitely support the rules allowing us to write the same thing in both places.

Changing existing .h files is not something worth spending resources on. Just get rid of the cognitive dissonance of "what's required HERE is forbidden THERE".

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADu_oUB%2B1DEsTpO6x6f0Cra8xmAi8G%3D__1_Qqky6WZk9bNK%3DVQ%40mail.gmail.com.

[Anita Woodruff]

Changing existing .h files is not something worth spending resources on. Just get rid of the cognitive dissonance of "what's required HERE is forbidden THERE".

+1. The more we can remove the hidden tripwires when working across chromium/blink code the better, imho.

 

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADu_oUB%2B1DEsTpO6x6f0Cra8xmAi8G%3D__1_Qqky6WZk9bNK%3DVQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAOqqYVGTheQ0N-jSpcsDJDWxWZ4hMhZf_WvwLCZRePosY%2BRgUw%40mail.gmail.com.

[TAMURA, Kent]

Emil objected the proposal.

So, I'd like to change the proposal so that we change the style rule to:

  *may* omit obvious parameter names from function declarations.

  However, recommend not to omit them for consistency with Google C++ style.

Any objection?

[Emil A Eklund]

I'm fine with that, thank you.