On Sun, Mar 05, 2017 at 10:04:01PM +0800, Christopher Li wrote:
> I am fine with the code logic change. However, I wish function has better names.
> It just looking at the function name, "external_decl" vs "external_declaration",
> it is hard to guess which function is lower level than the other.
Sure. I didn't liked it myself but at the moment I didn't got something
better. I'll try harder.
> > - }
> > + if (!is_typedef)
> > + process_decl(list, decl);
>
> I think here better check the process_decl is not NULL.
> The caller might issue NULL call back which means it is not
> interested in adding the symbol to a list.
Yes, we can and it's cheap but I think we'll always need a
callback here. And even if we don't, it's easy enough to provide
a callback that do nothing which avoid to always have to test the
pointer here to handle an hypothetical corner case.
> > +static void default_process_decl(struct symbol_list **list, struct symbol *decl)
> > +{
> > + if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
> > + add_symbol(list, decl);
> > + fn_local_symbol(decl);
> > + }
>
> Again, if I look at just pure function name "default_process_decl", it is
> very hard to me to guess what should happen in this function.
> After all, "process a declare" is very generic, it can mean any thing.
Yes, sure, but the keyword here is 'default' like in "don't care what this
is doing, it's the that's correct for most/normal situations".
> I suggest a more meaningful name related what the code does.
> e.g. "track_local_declaration". I don't know, I want to find a better name.
> You are welcome to suggest a better one.
I'll look for one.
> > +}
> > +
> > +struct token *external_declaration(struct token *token, struct symbol_list **list)
> > +{
> > + return external_decl(token, default_process_decl, list);
> > +}
>
> If I don't have a better function name for external_decl, I might just not use
> the wrapper function at all.
Yes, OK.
And to be honest, the current name 'external_declaration()' is far
from being a good name eithers: it doesn't give a hint on *what*
the function is doing and it's not only for 'extern' declaration.
Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
