Why commit 68404f13d4bf482?
Denys Vlasenko
vda.linux at googlemail.com
Mon Nov 1 00:30:18 UTC 2010
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
More information about the busybox
mailing list