All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Vesa Jääskeläinen" <chaac@nic.fi>
To: The development of GRUB 2 <grub-devel@gnu.org>
Subject: Re: Handlers
Date: Sat, 30 Aug 2008 17:55:19 +0300	[thread overview]
Message-ID: <48B95F57.2010907@nic.fi> (raw)
In-Reply-To: <87sksngumr.fsf@xs4all.nl>

Marco Gerards wrote:
> Vesa Jääskeläinen <chaac@nic.fi> writes:
> 
>> Marco Gerards wrote:
>>> If I knew a better way, I would have used it.  I think the
>>> alternatives will all end up in either code duplication or a loss of
>>> type safety that now is at least present through warnings.  But please
>>> understand that you only have to use the macros, not change them :-)
>>>
>>> Suggestions are welcome.
>> Hi,
>>
>> Well... How about something like this?
> 
> This code does not describe structs with function pointers but structs
> with data.  Furthermore, you made it very generic while you do need
> casts.  For example, you have a generic iterate function.  Except for
> the macros, this code is not too different.  But I used the macros to
> glue the list code to the other code of GRUB, which is what you left
> out.  But this is the essence of the discussion.
> 
>> It only requires one cast and no macros at all.
> 
> My code also contained generic code in kern/handler.c like your code.
> I have added the macros to avoid casts.  This code will not work
> directly in GRUB 2 for filesystems or whatever.  You need to cast at
> some points, or even quite often.  And that is *exactly* what I would
> like to avoid.

Hi,

But your code does the cast too :)... its just hidden inside macro
magic... Or did you lost track of it already as it was hidden ;) ;)

If you just paste code for GRUB_HANDLER_CREATE_C inside .c file we are
pretty much in the same functionality.

If we want to nitpick somewhere, your code assumes iterate makes a call
to int (*hook) (const grub_handler_t handler) where as actual function
pointer is something else.

Anyway... my point is. Do not hide code behind macros :). It just makes
life much harder when you are debugging or reading the code as you do
not see what is happening (and can even confuse debugger). You can do
generic register / unregister / iterate handling but keep it simple
stupid in code to make it easier to use in the end.

Thats my 2 cents on the issue.

Thanks,
Vesa Jääskeläinen




  reply	other threads:[~2008-08-30 14:55 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-08-29 12:36 Handlers Marco Gerards
2008-08-29 13:27 ` Handlers Neal H. Walfield
2008-08-29 14:46   ` Handlers Marco Gerards
2008-08-29 19:13     ` Handlers Neal H. Walfield
2008-08-29 14:58 ` Handlers Vesa Jääskeläinen
2008-08-29 15:10   ` Handlers Marco Gerards
2008-08-29 19:43     ` Handlers Vesa Jääskeläinen
2008-08-30  1:27       ` Handlers Marco Gerards
2008-08-30 14:55         ` Vesa Jääskeläinen [this message]
2008-08-30 12:45 ` Handlers Robert Millan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=48B95F57.2010907@nic.fi \
    --to=chaac@nic.fi \
    --cc=grub-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.