[U-Boot] [PATCH v9] Introduced btrfs file-system with btrload command

[Tom Rini]

On Wed, Mar 20, 2013 at 04:55:00PM +0000, Adnan Ali wrote:
> On 20/03/13 15:23, Tom Rini wrote:
> >On Wed, Mar 20, 2013 at 04:10:05PM +0100, Wolfgang Denk wrote:
> >>Dear Adnan Ali,
> >>
> >>In message <1363789411-9663-1-git-send-email-adnan.ali at codethink.co.uk> you wrote:
> >>>Introduces btrfs file-system to read file from
> >>>volume/sub-volumes with btrload command. This
> >>>implementation has read-only support.
> >>>This btrfs implementation is based on syslinux btrfs
> >>>code, commit 269ebc845ebc8b46ef4b0be7fa0005c7fdb95b8d.
> >>>
> >>>v8:     patch re-formated.
> >>>v7:     patch re-formated.
> >>>v6:     patch re-formated.
> >>What exactly is going on here?  Why do you have to go through so many
> >>iterations just reformatting again and again and again?
> >Yes, most of "patch re-formatted" really means "reworked for checkpatch
> >problems".
> >
> >>>+void btrfs_type(char num)
> >>>+{
> >>>+	switch (num) {
> >>>+	case BTRFS_FILE:
> >>>+		printf("<FILE>   "); break;
> >>>+	case BTRFS_DIR:
> >>>+		printf("<DIR>    "); break;
> >>>+	case BTRFS_SYMLNK:
> >>>+		printf("<SYM>    "); break;
> >>>+	default:
> >>>+		printf("<UNKNOWN>"); break;
> >>Can you please use puts() instead of print() for all output that does
> >>not really need any formatting?
> >Agreed (and with the other stuff I've snipped too, as those are
> >introduced).
>     Simon was happy with that after i changed all error messages to
>  debug. But i can change unformatted messages to puts. if everyone
> is agrees.

OK, so what we're saying is that here you should use puts() (which still
always prints, but is quicker when using a constant string with no
formatting characaters).

> >>>+		if (ret < 0)
> >>>+			low = mid + 1;
> >>>+		else if (ret > 0)
> >>>+			high = mid;
> >>>+		else {
> >>>+			*slot = mid;
> >>>+
> >>>+			return 0;
> >>>+		}
> >>Is this imported code?
> >Yes.
>    yes
> >
> >>>+	if (__le64_to_cpu(m1->logical) > __le64_to_cpu(m2->logical))
> >>>+
> >>>+		return 1;
> >>>+
> >>>+	if (__le64_to_cpu(m1->logical) < __le64_to_cpu(m2->logical))
> >>>+
> >>>+		return -1;
> >>Is this imported code? Otherwise: can we drop these empty lines before
> >>the returns?
>     It wasn't like that simon asked me to add line before return statement.
> He was happy with that too. Again i will remove it once everyone agreed
> it is the right way.

What Simon means (and the right style is):
if (foo)
	return;

if (bar)
	return;

I think what you had before was:
if (foo)
	return;
if (bar)
	return;

Or even:
if (foo) return;

> >This, and the rest are not.  But checkpatch.pl doesn't complain about
> >them either, annoyingly.  Adnan, try doing a diff between the syslinux
> >and u-boot files to look for other whitespace oddities that got
> >introduced.  Thanks.  And, thanks for fixing all of the problems
> >checkpatch does catch.
> >
>  According to checkpatch there are no whitespaces and errors.
> But what exactly are you after. Even though i have removed all
> errors and warnings from whole patch.

Yes, you've fixed everything checkpatch finds, so thanks.  There's a few
things however that checkpatch didn't catch such as using puts() when
you're printing strings and not using formatting characters or spacing
problems in Makefiles, or the if (...) blank line return construct.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130320/45f64f12/attachment.pgp>