On Mon, 2009-05-04 at 01:37 +0200, Javier Martín wrote: > > The patch adds many trailing spaces. I suggest that you run GNU indent > > on drivemap.c. It will take care of most of the trailing spaces. > > Comments will still need to be fixed. > > > > Assembler files use different formatting in GRUB. Also, it's better to > > use meaningful names for the labels. Label 4 is unused. > Where can I find those assembly formatting conventions? From what I see > in your version of the patch, the gist is that asm instructions start at > the 8th column (not after a \t). This carries an unpleasant > FORTRAN-esque smell that I would rather avoid.
I don't know if it's documented anywhere, but it's sufficient to looks at other *.S files in GRUB or another GNU project, such as GCC. > > Some comments are excessive or unnecessary. "These functions defined in > > this file may be called from C" - irrelevant, we have no such functions. > > Complaints that the processor is not in 64-bit mode are also useless. I > > don't understand what "bundle" means in the comments. > Sorry, I copied the initial comment from another .S file in GRUB2 as a > kind-of-boilerplate. In this context, "bundle" means the whole "int13 > handler" package that is deployed to the reserved memory address, > consisting of the old IVT pointer, the actual int13h handler code and > the entry map. It would be great if you change the comments to avoid the word "bundle" unless it's explained. > > What is "(void) mod;"? It doesn't prevent any warnings for me. > Once again, boilerplate code copied from hello.c long ago. I suspect > this stopped being required when the command framework was revamped. Correct. We are using gcc attributes to suppress warnings in GRUB_MOD_INIT. I've committed a patch that removes all that stuff. > > grub_symbol_t is already used in kern/dl.c with a different meaning. > > Why not just use void? > The reason to avoid using a plain "void" is that it might be a slightly > confusing sight, so a future coder might have an idea-of-the-moment and > change it to a "meaningful type" like void*. The presence of an explicit > type with a big comment is supposed to at least make people think twice > before changing it. You can leave the comment but use void. I don't think anyone (of the reasonable people allowed to touch GRUB code) would change the type if the code is working. > > Improved patch is attached. > Thanks. I will read it thoroughly tomorrow when I'm back from uni. I > think that drivemap.h could be scrapped, its contents incorporated into > drivemap.c so as to reduce clutter and bitrot potential, and would > reduce the impact of the declaration of grub_symbol_t even more. That's a good idea. Still, I would prefer that we don't use grub_symbol_t where void is be sufficient. -- Regards, Pavel Roskin _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
