Michal, All,
On 2013-07-08 19:35 +0200, Yann E. MORIN spake thusly:
> On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > > From: "Yann E. MORIN" <yann.morin.1998@xxxxxxx>
> [--SNIP--]
> > > Since the search can be a regexp, it is possible that more than one symbol
> > > matches exactly. In this case, we can't decide which to sort first, so we
> > > fallback to alphabeticall sort.
[--SNIP--]
> > > Reported-by: Jean Delvare <jdelvare@xxxxxxx>
> > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@xxxxxxx>
> > > Cc: Jean Delvare <jdelvare@xxxxxxx>
> > > Cc: Michal Marek <mmarek@xxxxxxx>
> > > Cc: Roland Eggner <edvx1@xxxxxxxxxxxxxxxxxx>
> > > Cc: Wang YanQing <udknight@xxxxxxxxx>
> >
> > I tested it and it works fine, thanks!
> >
> > Tested-by: Jean Delvare <jdelvare@xxxxxxx>
> >
> > Now comes my late review. Overall I like the idea and the code but a few
> > things could be improved:
>
> Since this is already in Michal's tree, on should I proceed?
> - send an updated patch that replaces that one, or
> - send another additional patch with your proposed changes?
OK, since Michal already sent his pull-request to Linus, I'll prepare a
corrective patch I'll submit before the end of the week. Is that OK with
you, Michal?
[--SNIP--]
> > > +static int sym_rel_comp( const void *sym1, const void *sym2 )
> > > +{
> > > + struct sym_match *s1 = *(struct sym_match **)sym1;
> > > + struct sym_match *s2 = *(struct sym_match **)sym2;
> >
> > You shouldn't need these casts.
>
> Probably not, indeed, but I like to write (and read) what I expect to
> happen, and pointer arithmetics is always something I dread to foobar.
In fact, we need the cast, otherwise gcc whines about const/non-const.
[--SNIP--]
> > > for_all_symbols(i, sym) {
> > > + struct sym_match *tmp_sym_match;
> > > if (sym->flags & SYMBOL_CONST || !sym->name)
> > > continue;
> > > - if (regexec(&re, sym->name, 0, NULL, 0))
> > > + if (regexec(&re, sym->name, 1, match, 0))
> > > continue;
> > > if (cnt + 1 >= size) {
> >
> > I think the "+ 1" can be dropped because the new array is not
> > NULL-terminated.
Indeed.
> > > + sym_match_arr = tmp;
> > > }
> > > sym_calc_value(sym);
> > > - sym_arr[cnt++] = sym;
> > > + tmp_sym_match = (struct sym_match*)malloc(sizeof(struct sym_match));
> >
> > Cast not needed.
>
> OK.
>
> > In fact I don't think this allocation is needed in the first place.
> > Calling malloc for every match is rather costly. If you would have
> > allocated an array of struct sym_match (rather than only pointers
> > thereto) before, you would not need this per-symbol malloc. Struct
> > sym_match is small enough to not warrant an extra level of allocation
> > and indirection IMHO.
Indeed, it makes for cleaner code.
Thank you again! :-)
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
