On 02/05/14 19:00, Marek Vasut wrote:
> On Wednesday, February 05, 2014 at 01:40:27 PM, Mateusz Zalega wrote:
>
> [...]
>
>>> Are these two new functions called from multiple places at all? If not,
>>> just inline these ll_foo() calls and be done with it. FYI you can also
>>> make macros for these to avoid having to type all these args all around
>>> and duplicating the code.
>>
>> Macros or static inlines, it's all the same
>
> NAK, what you say is seriously wrong, you should know that by now!
>
> Macros do not do any kind of typechecking, functions do typechecking.
> Macros are expanded in place during preprocessing, functions are (usually)
> single-instance.
>
> etc.
Yeah, and it's all the same if you don't care for typechecking and all
that, which I assumed from _you_ proposing usage of macros here.
>> there's no point in
>> changing that. The symbols aren't visible outside this compilation unit
>> and function calls are, well, inlined.
>
> It's pointless to have them pulled out so explicitly. Or would you prefer to
> have each function encapsulated in another function ? This doesn't make does
> now, does it ?
Pardon?
> What I would like to do is for you to follow the advice in linker_lists.h and
> produce a small shim over these crude linker lists prototypes there, so that
> the
> users here in the g_* world don't have to type the whole linker list macros
> with
> all the arguments, which do not ever change for this g_* . Does it make sense
> please?
It's taken care of by static inlines.
>>>> static int g_dnl_do_config(struct usb_configuration *c)
>>>> {
>>>>
>>>> const char *s = c->cdev->driver->name;
>>>>
>>>> - int ret = -1;
>>>>
>>>> debug("%s: configuration: 0x%p composite dev: 0x%p\n",
>>>>
>>>> - __func__, c, c->cdev);
>>>> -
>>>> + __func__, c, c->cdev);
>>>>
>>>> printf("GADGET DRIVER: %s\n", s);
>>>>
>>>> - if (!strcmp(s, "usb_dnl_dfu"))
>>>> - ret = dfu_add(c);
>>>> - else if (!strcmp(s, "usb_dnl_ums"))
>>>> - ret = fsg_add(c);
>>>> - else if (!strcmp(s, "usb_dnl_thor"))
>>>> - ret = thor_add(c);
>>>> -
>>>> - return ret;
>>>> +
>>>> + struct g_dnl_bind_callback *callback = g_dnl_first_bind_callback();
>>>> + for (; callback != g_dnl_last_bind_callback(); ++callback)
>>>
>>> callback++ , this is not C++ where the order might matter. Nonetheless,
>>> you do
>>
>> It doesn't matter anyway and can't be supported on Coding Style grounds,
>> don't bug me.
>
> Can be done on purely statistical grounds, try this:
>
> $ git grep 'for.*(.* *++[:alnum:]\+ *)' | wc -l
> 13
> $ git grep 'for.*(.* *[:alnum:]\+++ *)' | wc -l
> 183
>
> Please fix, thank you.
Okay, whatever.
>>> want to use ll_entry_count() and ll_entry_get() with an iterator variable
>>
>> I don't think using ll_entry_get() in this way is possible with current
>> implementation of linker lists. Moreover, there's plenty of code doing
>> just the same already accepted to U-Boot.
>
> Ah meh, sorry. Seems like someone was messing with the linkerlists and
> misdesigned it. Dang.
$ git show 42eba
Yeah, it's a pity.
>>> instead to make sure you don't step onto a corrupted field and crash in
>>> some weird way.
>>
>> Linker would have to split sections to make this sort of thing possible.
>> Won't happen.
>
> Can you please elaborate ?
> [...]
>
You're guaranteed by the linker, and our setup, that all your
linker-list data will end up in a contiguous block.
--
Mateusz Zalega
Samsung R&D Institute Poland
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
