[U-Boot] [RFC][PATCH] Code Clean-up (weak functions)

[Graeme Russ]

On Wed, Dec 24, 2008 at 12:14 PM, Shinya Kuribayashi
<shinya.kuribayashi at necel.com> wrote:
> Graeme Russ wrote:
>>

[Snip]

>>  - All weak functions are declared as __function() in the source file with
>>   funtion() __attribute__((weak, alias("function"))); on the line
>> immediately
>>   after the closing brace of __function() - for example:
>>     void __do_something (args)
>>     {
>>     ...some code...
>>     }
>>     do_something(args) __atttribute__((weak, alias("__do_something")));
>
> Why do we need to being consistent?  I don't see real/technical benefits
> to doing so.  Using alias or not should be each developer's business.  I
> completely disagree with forcing alias use here.
>

Consistent code is easier to maintain - If we eliminate inconsistencies
early, it stops them being spread and resulting in spaghetti code

[snip]

>> diff --git a/board/incaip/incaip.c b/board/incaip/incaip.c
>> index 3b30970..3ee3ac9 100644
>> --- a/board/incaip/incaip.c
>> +++ b/board/incaip/incaip.c
>> @@ -31,7 +31,7 @@
>>
>>  extern uint incaip_get_cpuclk(void);
>>
>> -void _machine_restart(void)
>> +void machine_restart(void)
>>  {
>>        *INCA_IP_WDT_RST_REQ = 0x3f;
>>  }
>
> Why change the function name?  It's derived from Linux/MIPS, and I'd
> like to keep consistent with with him.  Please don't touch it.

For consistency :) - But I 100% agree that if the code is coupled to another
source and meets that sources rules then it is easier and more maintainable
to keep their rules - I have no objection to retaining the original names

[snip]

>
>> diff --git a/cpu/mips/cpu.c b/cpu/mips/cpu.c
>> index b7180b0..84c4730 100644
>> --- a/cpu/mips/cpu.c
>> +++ b/cpu/mips/cpu.c
>> @@ -38,13 +38,13 @@
>>        :                                                               \
>>        : "i" (op), "R" (*(unsigned char *)(addr)))
>>
>> -void __attribute__((weak)) _machine_restart(void)
>> -{
>> -}
>> +void inline __machine_restart(void) {}
>> +void inline machine_restart (void)
>> +       __attribute__((weak, alias("__machine_restart")));
>>
>>  int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>>  {
>> -       _machine_restart();
>> +       machine_restart();
>>
>>        fprintf(stderr, "*** reset failed ***\n");
>>        return 0;
>
> Why inline?  It seems totally wrong to me.

I wasn't aiming to change the semantics of whether functions where inline or
not (only weak / aliased). I agree that inline does not make sense here

[snip]

>>
>> > > Notice that in Linux, the 'alias' construction is not being used
>> > massively. Can it be removed here also, or is it somehow mandatory
>> > here?
>>
>> I don't think it is mandatory but it is in the majority in u-boot.
>
> If it's not mandatory, please drop all aliases.  That saves source code
> size (not generated u-boot image size) a little bit.  Majority or not
> does not make sense here.
>

I wanted to make the impact of the patch as small as possible - I agree that
this would make the code cleaner, simpler and smaller but would require
more extensive testing across a lot of build-tools to make sure they all
fully support weak functions in this way. I am more than happy to create a
patch, but it will require a _lot_ of regression testing

Regards,

Graeme