grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Drivemap module


From: Pavel Roskin
Subject: Re: [PATCH] Drivemap module
Date: Thu, 12 Jun 2008 16:31:15 -0400

On Tue, 2008-06-10 at 23:31 +0200, Javier Martín wrote:

> Ok, I'm sorry and don't mean to be intrusive, I just thought the last
> messages might have got lost between mail filters - it's happened to me.

Here's my very superficial review.

Please don't add any trailing whitespace.  Line in the patch that start
with a plus should not end with a space or a tab.

Please avoid camelCase names, such as bpaMemInKb and retVal.  Local
variables should generally be short, like "ret" and "bpa_mem".

Some strings are excessively long.  While there may be exception of the
80 column limit, I see a 118 character long line that can be trivially
wrapped.

The patch add a new warning:

commands/i386/pc/drivemap_int13h.S: Assembler messages:
commands/i386/pc/drivemap_int13h.S:124: Warning: .space repeat count is
zero, ignored

I'm not sure what you meant there.

I don't think using #undef is a good idea.  It's better to use macro
names that would never be reused accidentally and thus never need to be
undefined.

-- 
Regards,
Pavel Roskin




reply via email to

[Prev in Thread] Current Thread [Next in Thread]