Why commit 68404f13d4bf482?

[Denys Vlasenko]

On Sunday 31 October 2010 21:02, Rob Landley wrote:
> Adding -wunused-parameter and then typecasting your way around it in dozens of 
> files not only bloats the code with lots of brittle pointless annotations, but 
> means you can't use that flag later to find potential optimizations where we 
> could try to remove the parameter from functions that don't necessarily need 
> it.

But I do use this flat exactly for this purpose! I add ATTRIBUTE_UNUSED
only when I definitely can't just get rid of the parameter
(for example, when the function has to match a certain prototype).

That commit did remove a lot of such unused parametes. One example:

-procps_status_t *alloc_procps_scan(int flags)
+static procps_status_t *alloc_procps_scan(void)
 {
        unsigned n = getpagesize();
        procps_status_t* sp = xzalloc(sizeof(procps_status_t));
@@ -175,7 +175,7 @@ procps_status_t *procps_scan(procps_status_t* sp, int flags)
        struct stat sb;

        if (!sp)
-               sp = alloc_procps_scan(flags);
+               sp = alloc_procps_scan();


> (Yeah, now we can grep for the annotation.)  Did it find a single actual  
> _bug_ in the code where we weren't using a parameter that we should have?  
> Isn't that what a test suite is for?

I found a lot of functions which genuinely did not need some parameters,
but the parameters were still evaluated and passed down.

Bloat.

In some cases, as a result there were cascading parameter removals:
a() -> b(param) -> c(param), c doesn't use the param, and then
I find out that b used it only to pass it down to c! Etc...

Compiler still finds new such cases when new stuff is added
or existing code is reworked.

> Adding this extra verbiage also means that if we try to get other compilers, 
> like LLVM, to build busybox, we have to go through extra steps to explicitly 
> add support to them.  It's a step _away_ from genericizing the code.

Yes.

> Plus it's one more thing to learn when reading the busybox code for the first 
> time.  I got into busybox development in the first place because the code was 
> much simpler and easier to read than other implementations.  Now, between all 
> the MAIN_EXTERNALLY_VISIBLE magic (why on earth are you prototyping a function 
> on the line above wher eyou declare it??!??)

Delete one such line, rebuild, and you will get a warning about
missing prototype. I can drop -Wmissing-prototypes, do you think
it is a right step?

MAIN_EXTERNALLY_VISIBLE thing is for libbusybox. libbusybox was exporting
tons of libbb names. This was not needed, and made libbusybox
bigger by several kilobytes.

Now it exports only APPLET_main names, but therefore they do need to be
flagged for export.

> and the UNUSED_PARAM stuff (which  
> apparently exists to make sure that the prototype and the declaration can't e 
> collapsed together into a single macro, but _must_ be typed out in full both 
> times and certified by a public notary)...

I didn't try. I think such macro can be made to work.

-- 
vda