Re: amd64 bitops fix for -Os

[Randy.Dunlap]

On Mon, 31 Oct 2005, Alexandre Oliva wrote:

> On Oct 31, 2005, Randy Dunlap <randy_d_dunlap@xxxxxxxxxxxxxxx> wrote:
>
> >> Signed-off-by: Alexandre Oliva <oliva@xxxxxxxxxxxxxxxxx>
>
> > Possibly Andrew or Andi have already merged this into their trees.
> > However, I have a few comments on the patch re Linux style.
> > They are meant to help inform you and others -- that's all.
>
> Thanks, I didn't realized I'd deviated from the recommended style.  In
> this updated version of the patch, I've removed the ifdefs that could
> sanity-check arguments to the exported entry points and adjusted the
> comments to follow the guidelines.
>
> >> --- arch/x86_64/lib/bitops.c~	2005-10-27 22:02:08.000000000 -0200
> >> +++ arch/x86_64/lib/bitops.c	2005-10-29 18:24:27.000000000 -0200
>
> > Diffs should start with a top-level names (even if it's entirely
> > phony), so that they can be applied with many scripts that are around
> > and expect that.
>
> I hope you mean -p1 vs -p0.  I tend to prefer -p0 myself, but quilt
> makes it easy enough to handle either :-)  Fixed in the revised
> version.

Yes, that's all that I meant.

> >> -inline long find_first_zero_bit(const unsigned long * addr, unsigned long size)
> >> +static inline long
> >> +__find_first_zero_bit(const unsigned long * addr, unsigned long size)
>
> > The only good reason for splitting a function header is if it would
> > otherwise be > 80 columns, not just to put the function name at the
> > beginning of the line.
>
> In this case, it would, because I'm adding static and two leading
> underscores.  But I'll keep that in mind, since this is quite
> different from the GCC style.
>
> >> +		 /* Any register here would do, but GCC tends to
> >> +		    prefer rbx over rsi, even though rsi is readily
> >> +		    available and doesn't have to be saved.  */
> >> +		 [addr] "S" (addr) : "memory");
>
> > Comment in the middle of the difficult-to-read asm instruction in
> > undesirable (IMO).
>
> I hope it is as useful after the statement.  Moving it before it would
> move it too far apart from what it refers to IMHO.
>
> Thanks again for the style feedback, it's really appreciated.

and thanks for the changes too.

> Should I have retained the problem description in the patch file, or
> is the Signed-off-by: line enough?

There should be a short problem description in the patch
(before the SOB: line).

See Documentation/SubmittingPatches
or <http://linux.yyz.us/patch-format.html>
or <http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt> .

-- 
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/