On Wed, Dec 24, 2008 at 12:14 PM, Shinya Kuribayashi <shinya.kuribaya...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot