[Freeciv-Dev] Re: (PR#8504) Add function to get all section entries

["Raimar Falke" <i-freeciv-lists@xxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=8504 >

On Thu, Apr 15, 2004 at 12:58:35PM -0700, Jason Short wrote:
> 
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=8504 >
> 
> Raimar Falke wrote:
> > <URL: http://rt.freeciv.org/Ticket/Display.html?id=8504 >
> > 
> > 
> > For things like
> > 
> > [key_bindings]
> > c="center_unit"
> > C="center_capital"
> > 
> > Kp8="move_unit(n)"
> > Kp6="move_unit(e)"
> > Kp2="move_unit(s)"
> > Kp4="move_unit(w)"
> > 
> > Kp7="move_unit(nw)"
> > Kp9="move_unit(ne)"
> > Kp3="move_unit(se)"
> > Kp1="move_unit(sw)"
> > 
> > Left="scroll(left,1,tile)"
> > Right="scroll(right,1,tile)"
> > Up="scroll(up,1,tile)"
> > Down="scroll(down,1,tile)"
> > 
> > Shift-Left="scroll(left,0.5,screen)"
> > Shift-Right="scroll(right,0.5,screen)"
> > Shift-Up="scroll(up,0.5,screen)"
> > Shift-Down="scroll(down,0.5,screen)"
> > 
> > Alt-Left="scroll(left,1,pixel)"
> > Alt-Right="scroll(right,1,pixel)"
> > Alt-Up="scroll(up,1,pixel)"
> > Alt-Down="scroll(down,1,pixel)"
> > 
> > I need a way to iterate over all keys of the given section. The
> > attached patch add such a function.
> 
> I'm pretty sure you don't *need* this.  Instead of doing
> 
>    entries = secfile_get_section_entries(file, "key_bindings", &n);
> 
>    for (i = 0; i < n; i++) {
>      interpret_key_binding(entries[i]);
>    }
>    free(entries);
> 
> You could do
> 
>    for (key = 0; key < NUM_KEYS; i++) {
>      char *keyname = keys[key];
>      char *keyval = secfile_lookup_string_default(file, NULL, ...);
> 
>      if (keyval) {
>        interpret_key_binding(keyval);
>      }
>    }
> 
> this might be more reliable, but may also be more work (you need a list 
> of keys

Yes it could be done this way. However there are a lot of keys (8 *
(2*26+2*10+n)) = at least 576.

> - but you probably need this list anyway...).

No I don't.

> Anyway, the function sounds like it is generally useful.

It is.

> > +  section_list_iterate_rev(*my_section_file->sections, asection) {
> > +    if (strcmp(asection->name, section) == 0) {
> > +      psection = asection;
> > +      break;
> > +    }
> > +  } section_list_iterate_rev_end;
> 
> This should become a helper function.  There's code just like this in 
> several places (lines 418, 1146, 1202).

Ok.

> Why do you use section_list_iterate_rev instead of section_list_iterate 
> like everyone else?  The only other place section_list_iterate_rev is 
> used is for adding to a secfile, on the assumption that the last-entered 
> section is most likely to be added to again.  This doesn't apply when 
> reading a file.

You now know from which place I copied the lines ;) I haven't paid
attention.

> > +      *num=0;
> 
> Bad style.

Why?

> > +  ret = fc_malloc((*num) * sizeof(char *));
> 
> Should be sizeof(*ret)

Yes.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
    1) Customers cause problems.
    2) Marketing is trying to create more customers.
  Therefore:
    3) Marketing is evil.