[patch #3330] HURD_IHASH_ITERATE_KEYS

[Marcus Brinkmann]

This mail is an automated notification from the patch tracker
 of the project: The GNU Hurd.

/**************************************************************************/
[patch #3330] Latest Modifications:

Changes by: 
                Marcus Brinkmann <marcus@gnu.org>
'Date: 
                Mon 11/08/04 at 16:08 (Europe/Berlin)

------------------ Additional Follow-up Comments ----------------------------
To Neal, the current form exists because most of the time you don't need the 
variable outside of the scope, and you can always achieve what you want with 
something like { outside_val = val; break; } in your code.

OTOH, I see your and Ogi's point.  Macro tricks are not particularly readable 
either, so defining them outside of the macro is actually something that is 
more natural.  I am starting to waiver.

This would call for a new patch, with only one macro, not two, that takes the 
name of already defined key and value variables, and just uses those.  This 
would also make the for loop much simpler, I guess, and remove all my terribly 
clever but not at all too readable hacks from this macro.

All callers need to be changed, too.







/**************************************************************************/
[patch #3330] Full Item Snapshot:

URL: <http://savannah.gnu.org/patch/?func=detailitem&item_id=3330>
Project: The GNU Hurd
Submitted by: Ognyan Kulev
On: Wed 09/01/04 at 07:18

Category:  libihash
Priority:  3 - Low
Resolution:  None
Privacy:  Public
Assigned to:  marcus
Originator Email:  
Status:  Open


Summary:  HURD_IHASH_ITERATE_KEYS

Original Submission:  New for loop that loops only once is used.  Its purpose 
is to define key as hurd_ihash_key_t.

To loop once, another variable first_loop is used.  Even in -O1, disassembled 
source shows that GCC correctly optimizes the case and the variable is not 
defined at all.

Follow-up Comments
------------------


-------------------------------------------------------
Date: Mon 11/08/04 at 16:08         By: Marcus Brinkmann <marcus>
To Neal, the current form exists because most of the time you don't need the 
variable outside of the scope, and you can always achieve what you want with 
something like { outside_val = val; break; } in your code.

OTOH, I see your and Ogi's point.  Macro tricks are not particularly readable 
either, so defining them outside of the macro is actually something that is 
more natural.  I am starting to waiver.

This would call for a new patch, with only one macro, not two, that takes the 
name of already defined key and value variables, and just uses those.  This 
would also make the for loop much simpler, I guess, and remove all my terribly 
clever but not at all too readable hacks from this macro.

All callers need to be changed, too.


-------------------------------------------------------
Date: Wed 09/01/04 at 13:28         By: Ognyan Kulev <ogi>
I don't need such macro for the ext2fs patch.  But I needed one for ext3fs.  
Fortunately, in my case, I could retrieve key from value.

Neal, I forgot about the break requirement.  I was too busy to think about the 
block requirement ;-)

Requiring KEY and VALUE to be defined by user seems OK to me: it looks 
"natural" and GCC will optimize when one of them is not used.

-------------------------------------------------------
Date: Wed 09/01/04 at 13:17         By: Neal H. Walfield <neal>
Marcus, a single macro (which provides both the key and the value)
is also a good solution.  I don't think the
IHASH...(..., hurd_ihash_value_t val) form is terrible useful, however,
as we already require c99 and in which case you can just do:

    ...
    hurd_ihash_value_t val;
    IHASH...(..., val);

(i.e. you don't even need an enclosing block).

Thanks,
Neal


-------------------------------------------------------
Date: Wed 09/01/04 at 12:19         By: Marcus Brinkmann <marcus>
Ogi, do you actually need this in your code?

In general, I don't object to make the key available, although if we do it, we 
could just as well do it always in a single macro, and we don't need two of 
them.

I'd still like to somehow retain the ability to do it in a local variable.  I 
am thinking about something like IHASH...(..., val) if you want the outside 
scope and IHASH...(..., hurd_ihash_value_t val) if you want a local variable, 
but I am not sure this can be achieved while at the same time retaining the 
block and break semantics.


-------------------------------------------------------
Date: Wed 09/01/04 at 10:57         By: Neal H. Walfield <neal>
I agree this function is useful, however, I think the patch is
incorrect.  Marcus was very careful to make sure that a break in
HURD_IHASH_ITERATE would really break out of the iteration.  We should
strive to preserve this functionality in HURD_IHASH_ITERATE_KEYS.

The problem, of course, is that we can only declare a variables of a
single type in a for loop initializer section and Marcus automatically
declares the VAL variable for the user.  I think this is actually
detrimental as then it becomes inaccessible outside of the loop and
makes things like this impossible:

        hurd_ihash_t ht;
        HURD_IHASH_ITERATE (ht, val)
          if (condition1_p (val) || condition2_p (val))
            break;

        ...

        /* Examine val.  */

So, I think we should have the user declare VAL (and in the case of
your extension, KEY).

I have attached a patch which does this.  It updates libihash
but not the rest of the users.  If Marcus agrees then I can make a
patch to do this.

Here is a test case:

#include <stdio.h>
#include <ihash.h>

int
main (int argc, char *argv[])
{
  hurd_ihash_t ht;

  hurd_ihash_create (&ht, HURD_IHASH_NO_LOCP);

  hurd_ihash_add (ht, (hurd_ihash_key_t) 2, (hurd_ihash_value_t) -2);
  hurd_ihash_add (ht, (hurd_ihash_key_t) 3, (hurd_ihash_value_t) -3);
  hurd_ihash_add (ht, (hurd_ihash_key_t) 4, (hurd_ihash_value_t) -4);

  hurd_ihash_key_t key;
  hurd_ihash_value_t val;

  HURD_IHASH_ITERATE(ht, val)
    printf ("%dn", (int) val);

  HURD_IHASH_ITERATE_KEY(ht, key, val)
    printf ("%d -> %dn", (int) key, (int) val);

  return 0;
}







File Attachments
-------------------

-------------------------------------------------------
Date: Wed 09/01/04 at 10:57  Name: ihash.diff  Size: 4.67KB   By: neal
Refactor HURD_IHASH_ITERATE; use a single for loop
http://savannah.gnu.org/patch/download.php?item_id=3330&amp;item_file_id=3627

-------------------------------------------------------
Date: Wed 09/01/04 at 07:18  Name: ihash.patch  Size: 900B   By: ogi

http://savannah.gnu.org/patch/download.php?item_id=3330&amp;item_file_id=3625






For detailed info, follow this link:
<http://savannah.gnu.org/patch/?func=detailitem&item_id=3330>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/