Re: Generic bootinfo implementation

[Matthieu Lemerre]

Some comments on your work (note that I'm far from being a Hurd coding
standards expert):
*I don't think we use the
/* 
 * Implemented types 
 */
 comment style, even as "titles" for part of files.  Just put /*
 Implemented types.  */.  Don't forget the dot at the end of your
 sentences and the two spaces in your comments.
 You can also separate different parts of a file by putting ^L
 characters into it (C-q C-l) with Emacs.  This enable to use the
 forward-page/backward-page commands in Emacs. (Little advertisement :) : I
 enhanced a page-break-mode for emacs which display a nice line instead
 of the ^L, on http://www.emacswiki.org/cgi-bin/emacs-fr.pl/PageBreaks)
*You put extra parenthesis, like in
 (l4_generic_bootinfo_t) (l4_boot_info());.  These are not needed.
*Try to avoid lines longer than 80 columns. Less than 78 columns is
  even better.
*We prefer not to align the = vertically.
*Try to avoid too much typecasts.  For instance, you declare
     l4_word_t br = (l4_word_t) l4_generic_bootinfo_first_entry (bi)
  and then you either systematically typecast br to a
 (l4_generic_boot_rec_t) or to a (l4_generic_bootinfo_module_t). It
 would be much simpler to write:
     l4_generic_boot_rec_t br = l4_generic_bootinfo_first_entry (bi);
     l4_generic_bootinfo_module_t bootinfo_module =
 (l4_generic_bootinfo_module_t) br;
or something like this (I don't know if br is even needed; the
offset_next and types fields are common to both structures).
*Don't declare your functions in source file:
      extern l4_word_t mbi_to_generic_bootinfo (multiboot_info_t*);
 because that make softwares harder to maintain. Put them in a header
 file (or create one).
*The bootinfo.h files look fine, but could you implement the L4 compat
 interface too?  I think it's just a matter of query-replace-regexp :)
*Don't use "generic" libl4 names like _L4_BOOTINFO_MODULE, but define
 for them a GNU and a L4 compat name.
*Please provide a Changelog entry along with your patches (but not as
 part of your patches). It's easier then to find out what you did, and
 to find bugs later.
Don't be scared by all the observations I made here! Many of them are
just matter of coding taste. Everything you wrote seems semantically
fine, even if I did not read everything carefully yet. It will be
easier to understand once you've supplied the Changelog entries :)
I don't know if you should redo your patch, because I don't know if we
want it to be integrated (nothing to do with you, just I don't know if
we need it). Well I guess the bootinfo part can still be integrated in
libl4. Marcus will be able to tell more on this, I hope.
If so, I think there is plenty of other things to do that we will be
happy to give you. 
Note that we require that you assign your changes to the FSF. So please
send an email to address@hidden so that we can incorporate them.
Hoping to talk to you soon,
Thanks,
Matthieu
Date: Mon, 21 Mar 2005 22:38:58 +0000
In-Reply-To: <address@hidden> (Alexandre Buisse's
 message of "Mon, 28 Feb 2005 23:02:24 +0100")
Message-ID: <address@hidden>
User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.0.50 (gnu/linux)