Possible false positive in checkpatch

Possible false positive in checkpatch

[Alan Stern]

The following appears to be a false positive in checkpatch:

ERROR: space prohibited after that '*' (ctx:BxW)
#163: FILE: drivers/usb/core/usb.c:304:
+#define usb_device_pm_ops      (* (struct pm_ops *) 0)
                                 ^

Certainly this is a rather uncommon code construction, but similar
ones might occur elsewhere.  To my eyes,

	(* (type *) ptr)

looks better than

	(*(type *) ptr)

or

	(*(type *)ptr)

or even

	(*(type*)ptr)

but of course this is a matter of opinion.  Is there any strong feeling 
about this in the kernel community?

Alan Stern

Re: Possible false positive in checkpatch

[Krzysztof Halasa]

Alan Stern <stern@rowland.harvard.edu> writes:

> ERROR: space prohibited after that '*' (ctx:BxW)


> Certainly this is a rather uncommon code construction, but similar
> ones might occur elsewhere.  To my eyes,
>
> 	(* (type *) ptr)
>
> looks better than
>
> 	(*(type *) ptr)
>
> or
>
> 	(*(type *)ptr)
>
> or even
>
> 	(*(type*)ptr)
>
> but of course this is a matter of opinion.  Is there any strong feeling 
> about this in the kernel community?

I think checkpatch already has gone way too far with this (and not
only this).

"type *var" vs "type* var" - sure, the latter is worse and provokes
"type* var1, var2", but anything else is IMHO only annoying and,
actually, not important WRT readability at all.

For example I prefer "type* func()" - as it's a function returning
"a pointer to type" and not "a pointer to a function returning type"
(which "type *func()" may suggest). Yes, func is not a pointer, so why
write "*" next to it?
-- 
Krzysztof Halasa

Re: Possible false positive in checkpatch

[Andy Whitcroft]

On Tue, Aug 12, 2008 at 05:29:02PM +0200, Krzysztof Halasa wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > ERROR: space prohibited after that '*' (ctx:BxW)
> 
> 
> > Certainly this is a rather uncommon code construction, but similar
> > ones might occur elsewhere.  To my eyes,
> >
> > 	(* (type *) ptr)
> >
> > looks better than
> >
> > 	(*(type *) ptr)
> >
> > or
> >
> > 	(*(type *)ptr)
> >
> > or even
> >
> > 	(*(type*)ptr)
> >
> > but of course this is a matter of opinion.  Is there any strong feeling 
> > about this in the kernel community?
> 
> I think checkpatch already has gone way too far with this (and not
> only this).
> 
> "type *var" vs "type* var" - sure, the latter is worse and provokes
> "type* var1, var2", but anything else is IMHO only annoying and,
> actually, not important WRT readability at all.
> 
> For example I prefer "type* func()" - as it's a function returning
> "a pointer to type" and not "a pointer to a function returning type"
> (which "type *func()" may suggest). Yes, func is not a pointer, so why
> write "*" next to it?

The recommendations it makes match the style of the whole, which new
contributions should follow.  To a lot of people these nuances don't
matter to others they do.  checkpatch aims to tell you what you will
likely be picked up on.  Its recommending a standardised style that is
not necessarily what any one of us would use.  But that is its role.
Feel free to ignore any of its recommendations, but expect to be pulled
up on a lot of them if you do; remembering the time of the reviewer
that is wasted in doing so.

-apw

Re: Possible false positive in checkpatch

[Krzysztof Halasa]

Andy Whitcroft <apw@shadowen.org> writes:

> The recommendations it makes match the style of the whole, which new
> contributions should follow.

But there isn't any "style of the whole". Existing styles differ.
While I guess there is some agreement about the base (tab width,
mostly K&R, don't do "char* ptr" etc.), the rest are simply views of
single persons.
This is IMHO fine as long as it's useful for the community, but not
past this point.
-- 
Krzysztof Halasa

Re: Possible false positive in checkpatch

[H. Peter Anvin]

Alan Stern wrote:
> The following appears to be a false positive in checkpatch:
> 
> ERROR: space prohibited after that '*' (ctx:BxW)
> #163: FILE: drivers/usb/core/usb.c:304:
> +#define usb_device_pm_ops      (* (struct pm_ops *) 0)
>                                  ^
> 
> Certainly this is a rather uncommon code construction, but similar
> ones might occur elsewhere.  To my eyes,
> 
> 	(* (type *) ptr)
> 
> looks better than
> 
> 	(*(type *) ptr)
> 
> or
> 
> 	(*(type *)ptr)
> 
> or even
> 
> 	(*(type*)ptr)
> 
> but of course this is a matter of opinion.  Is there any strong feeling 
> about this in the kernel community?
> 

Personally, I rather strongly prefer (*(type *)ptr).

	-hpa

Re: Possible false positive in checkpatch

[Alan Stern]

On Fri, 15 Aug 2008, H. Peter Anvin wrote:

> Alan Stern wrote:
> > The following appears to be a false positive in checkpatch:
> > 
> > ERROR: space prohibited after that '*' (ctx:BxW)
> > #163: FILE: drivers/usb/core/usb.c:304:
> > +#define usb_device_pm_ops      (* (struct pm_ops *) 0)
> >                                  ^
> > 
> > Certainly this is a rather uncommon code construction, but similar
> > ones might occur elsewhere.  To my eyes,
> > 
> > 	(* (type *) ptr)
> > 
> > looks better than
> > 
> > 	(*(type *) ptr)
> > 
> > or
> > 
> > 	(*(type *)ptr)
> > 
> > or even
> > 
> > 	(*(type*)ptr)
> > 
> > but of course this is a matter of opinion.  Is there any strong feeling 
> > about this in the kernel community?
> > 
> 
> Personally, I rather strongly prefer (*(type *)ptr).

It's probably safe to say that this is one of those gray areas where 
one need not adhere strictly to checkpatch's recommendations.

Alan Stern