patchThe GNU Hurd - Patches: patch #3330, HURD_IHASH_ITERATE_KEYS

 
 

You are not allowed to post comments on this tracker with your current authentification level.

patch #3330: HURD_IHASH_ITERATE_KEYS

Submitted by:  Ognyan Kulev <ogi>
Submitted on:  Wed 01 Sep 2004 07:18:54 AM UTC  
 
Category: libihashPriority: 3 - Low
Status: NonePrivacy: Public
Assigned to: Marcus Brinkmann <marcus>Open/Closed: Open
Planned Release: None
Wiki-like text discussion box: 

Mon 08 Nov 2004 04:08:43 PM UTC, comment #5:

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.

Marcus Brinkmann <marcus>
Project AdministratorIn charge of this item.
Wed 01 Sep 2004 01:28:24 PM UTC, comment #4:

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.

Ognyan Kulev <ogi>
Project Member
Wed 01 Sep 2004 01:17:14 PM UTC, comment #3:

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

Neal H. Walfield <neal>
Project Administrator
Wed 01 Sep 2004 12:19:54 PM UTC, comment #2:

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.

Marcus Brinkmann <marcus>
Project AdministratorIn charge of this item.
Wed 01 Sep 2004 10:57:31 AM UTC, comment #1:

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 ("%d\n", (int) val);

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

return 0;
}

Neal H. Walfield <neal>
Project Administrator
Wed 01 Sep 2004 07:18:54 AM UTC, 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.

Ognyan Kulev <ogi>
Project Member

 

Attached Files
file #7823:  ihash.diff added by neal (5kB - text/x-patch - Refactor HURD_IHASH_ITERATE; use a single for loop)
file #7821:  ihash.patch added by ogi (900B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

Do you think this task is very important?
If so, you can click here to add your encouragement to it.
This task has 0 encouragements so far.

Only logged-in users can vote.

 

Please enter the title of George Orwell's famous dystopian book (it's a date):

 

 

Follow 3 latest changes.

Date Changed By Updated Field Previous Value => Replaced By
Wed 01 Sep 2004 10:57:31 AM UTCnealAssigned toNone=>marcus
  Attached File-=>Added ihash.diff, #3627
Wed 01 Sep 2004 07:18:54 AM UTCogiAttached File-=>Added ihash.patch, #3625

Back to the top


Powered by Savane 3.1-cleanup