[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v9] block/raw-posix.c: Make physica
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v9] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host |
Date: |
Mon, 30 Nov 2015 17:26:39 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 30.11.2015 um 17:19 hat Eric Blake geschrieben:
> On 11/27/2015 12:35 PM, Programmingkid wrote:
>
> >> Unusual indentation; more typical is:
> >>
> >> | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
> >> *mediaIterator,
> >> | char *mediatType)
> >
> > I agree. I wanted the second long to be right justified with the 80
> > character line count.
>
> No. We don't right-justify code to 80 columns. That's not how it is
> done. Trying to do it just makes you look like the proverbial 'kid' in
> your pseudonym, rather than an adult to be taken seriously.
>
> Really, PLEASE follow the indentation patterns of the rest of the code
> base - where continued lines are left-justified to be underneath the
> character after (, and NOT right-justified to 80 columns. Violating
> style doesn't make your code invalid, but does make your patches less
> likely to be applied.
>
>
> >>> + /* If you found a match, leave the loop */
> >>> + if (*mediaIterator != 0) {
> >>> + DPRINTF("Matching using %s\n", matching_array[index]);
> >>> + snprintf(mediaType, strlen(matching_array[index])+1, "%s",
> >>
> >> Spaces around binary '+'.
> >
> > What's wrong with no spaces around the plus sign?
>
> Again, the prevailing conventions in the code base is that you put
> spaces around every binary operator. Yes, there is existing old code
> that does not meet the conventions, but it is not an excuse to add new
> code that is gratuitously different.
>
> >
> >>
> >>> + /* if a working partition on the device was not found */
> >>> + if (partition_found == false) {
> >>> + error_setg(errp, "Error: Failed to find a working partition on "
> >>> +
> >>> "disc!\n");
> >>
> >> and I already pointed out on v8 that this is not the correct usage of
> >> error_setg(). So, here's hoping v10 addresses the comments here and
> >> elsewhere.
> >
> > Kevin Wolf wanted it this way. What would you do instead?
>
> Keven and I both want you to use error_setg(), but to use it correctly -
> and the correct way is to NOT supply a trailing \n.
Nor leading "Error:", for that matter.
Kevin
pgp4F2QULBsIQ.pgp
Description: PGP signature