patchThe GNU Hurd - Patches: patch #3330, HURD_IHASH_ITERATE_KEYS

 
 

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

patch #3330: HURD_IHASH_ITERATE_KEYS

Submitter:  Ognyan Kulev <ogi>
Submitted:  Wed 01 Sep 2004 07:18:54 AM UTC
   
 
Category:  libihash Priority:  3 - Low
Status:  None Privacy:  Public
Assigned to:  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>
Group administrator
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>
Group 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>
Group 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>
Group administrator
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>
Group 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>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #7823:  ihash.diff added by neal (5KiB - 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

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

 

Follow 3 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2004-09-01 neal Assigned toNone marcus
    Attached File- Added ihash.diff, #3627
2004-09-01 ogi Attached File- Added ihash.patch, #3625

Back to the top

Powered by Savane 3.13-f8d8.
Corresponding source code