linux-kernel - Re: [PATCH] greybus: Fix coding stye error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170127135315.GI3560@localhost>
Date:   Fri, 27 Jan 2017 14:53:15 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Johan Hovold <johan@...nel.org>, mrossibellom@...il.com,
        elder@...nel.org, gregkh@...uxfoundation.org,
        greybus-dev@...ts.linaro.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] greybus: Fix coding stye error

On Fri, Jan 27, 2017 at 04:24:47PM +0300, Dan Carpenter wrote:
> On Fri, Jan 27, 2017 at 02:08:06PM +0100, Johan Hovold wrote:
> > On Fri, Jan 27, 2017 at 03:47:21PM +0300, Dan Carpenter wrote:
> > > The name sucks is what I'm saying.  Please fix it eventually.
> > 
> > I disagree. The name uses a common prefix that reflects the object it is
> > working on. This should not be changed. As it is currently named, the
> > function is also self-documenting. Trying to abbreviate the name just to
> > meet a 80 col limit (when there are alternatives) is just silly.
> 
> gb_interface_request_mode_switch()
> 
> It's not self documenting because there are no verbs in that sentence.

"request" is a verb.

We're requesting that the remote interface initiates a mode switch
(something which it can refuse).

It's a function that is only a called a couple of times. No need to
rewrite it as gb_intf_rms() or such (even if gb_intf would be a
reasonable prefix).

You comments about missing verbs etc make me wonder how you parse the
name:

	gb_interface		- a common prefix, indicating object
				  being acted on
	request_mode_switch 	- describes what is done

we're not fetching anything as your "gb_get_mode_switch()" suggestion
indicates you read it as.

> I guess in JAVA it would be considered good style.
> 
> This is "Enterprise Quality" in the derogatory sense of the term.  The
> prefix makes everything too long and doesn't add any value.  We don't
> keep the 80 character limit because of small screens, we keep it to
> discourage this type of code.

And to keep indentation down.

The kernel already has similar function names such as
"usb_driver_set_configuration" or "pm_runtime_mark_last_busy" which
follow the same pattern (prefix - verb - object).

Johan