[patch] remove a few unnecessary #includes
Bernhard Fischer
rep.nop at aon.at
Sat Sep 10 21:07:09 UTC 2005
On Sat, Sep 10, 2005 at 10:11:15PM +0200, Rainer Weikusat wrote:
>Bernhard Fischer <rep.nop at aon.at> writes:
>
>[...]
>
>> diff -X excl -rduNp busybox.busybox.oorig/archival/bunzip2.c busybox.busybox/archival/bunzip2.c
>> --- busybox.busybox.oorig/archival/bunzip2.c 2005-09-02 09:45:28.000000000 +0200
>> +++ busybox.busybox/archival/bunzip2.c 2005-09-09 22:13:12.000000000 +0200
>> @@ -6,7 +6,6 @@
>> */
>>
>> #include <fcntl.h>
>> -#include <getopt.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>> @@ -35,7 +34,7 @@ int bunzip2_main(int argc, char **argv)
>> src_fd = STDIN_FILENO;
>> filename = 0;
>> }
>> -
>> +
>> /* if called as bzcat force the stdout flag */
>> if ((opt & BUNZIP2_OPT_STDOUT) || bb_applet_name[2] == 'c')
>> filename = 0;
>
>That is not a very sensible patch.
It removes an unneeded include and deletes superfluous whitespace.
>
>> diff -X excl -rduNp busybox.busybox.oorig/archival/cpio.c busybox.busybox/archival/cpio.c
>> --- busybox.busybox.oorig/archival/cpio.c 2005-04-01 22:05:36.000000000 +0200
>> +++ busybox.busybox/archival/cpio.c 2005-09-09 22:11:37.000000000 +0200
>> @@ -24,20 +24,19 @@
>> *
>> */
>> #include <fcntl.h>
>> -#include <getopt.h>
>> #include <stdlib.h>
>> #include <string.h>
>> #include <unistd.h>
>> #include "unarchive.h"
>> #include "busybox.h"
>>
>> -#define CPIO_OPT_EXTRACT 0x01
>> -#define CPIO_OPT_TEST 0x02
>> -#define CPIO_OPT_UNCONDITIONAL 0x04
>> -#define CPIO_OPT_VERBOSE 0x08
>> -#define CPIO_OPT_FILE 0x10
>> +#define CPIO_OPT_EXTRACT 0x01
>> +#define CPIO_OPT_TEST 0x02
>> +#define CPIO_OPT_UNCONDITIONAL 0x04
>> +#define CPIO_OPT_VERBOSE 0x08
>> +#define CPIO_OPT_FILE 0x10
>> #define CPIO_OPT_CREATE_LEADING_DIR 0x20
>> -#define CPIO_OPT_PRESERVE_MTIME 0x40
>> +#define CPIO_OPT_PRESERVE_MTIME 0x40
>
>Before you changed the file, the constants where 'properly aligned'.
>Now, they aren't anymore. The precise point of this is (except
>damaging readability?)?
They still are. Your vi seems to be broken if they aren't aligned
properly after this patch.
Perhaps try
echo '
/*
Local Variables:
c-file-style: "linux"
c-basic-offset: 4
tab-width: 4
End:
*/
' >> archival/cpio.c
>
>>
>> extern int cpio_main(int argc, char **argv)
>> {
>> @@ -59,7 +58,7 @@ extern int cpio_main(int argc, char **ar
>> }
>>
>> if (opt & CPIO_OPT_TEST) {
>> - /* if both extract and test option are given, ignore extract option */
>> + /* if both extract and test options are given, ignore extract option */
>
>This adds a plural s where none is grammatically needed. This is
>called an ellipsis ("if both extract option and test option are
>given") and usually considered to be good style.
Ok. wasn't aware of that, sorry.
>
>> @@ -79,7 +78,7 @@ extern int cpio_main(int argc, char **ar
>> archive_handle->action_header = header_list;
>> }
>> }
>> - if (cpio_filename) { /* CPIO_OPT_FILE */
>> + if (cpio_filename) { /* CPIO_OPT_FILE */
>
>This appears to change what was a HTAB (\x9) into a space (\x20).
>The point is?
The tab looked ugly.
>
>[...]
>
>> diff -X excl -rduNp busybox.busybox.oorig/coreutils/env.c busybox.busybox/coreutils/env.c
>> --- busybox.busybox.oorig/coreutils/env.c 2005-09-05 21:44:17.000000000 +0200
>> +++ busybox.busybox/coreutils/env.c 2005-09-10 13:13:58.000000000 +0200
>> @@ -47,7 +47,7 @@
>> #include <stdlib.h>
>> #include <errno.h>
>> #include <unistd.h>
>> -#include <getopt.h>
>> +#include <getopt.h> /* struct option */
>> #include "busybox.h"
>
>... which translates to "ouch -- this wasn't superfloous after all --
Exactly. The next one who will look if there are any getopt calls left
doesn't need to open the file to see that it uses that struct.
Why is it bad to have that information at hand quickly ?
>better add a note before I delete it again accidentally". Not a
>sensible addition (either, comment all include or don't comment
>includes).
>
>> -#include <getopt.h>
>> +#include <getopt.h> /* optind */
>
>You should be using unistd.h for that (if only to pose as someone
>caring for portability).
I don't understand what you mean here, please explain.
>
>> diff -X excl -rduNp busybox.busybox.oorig/debianutils/readlink.c busybox.busybox/debianutils/readlink.c
>> --- busybox.busybox.oorig/debianutils/readlink.c 2005-04-01 22:05:37.000000000 +0200
>> +++ busybox.busybox/debianutils/readlink.c 2005-09-10 13:23:25.000000000 +0200
>> @@ -43,8 +43,6 @@ int readlink_main(int argc, char **argv)
>> RESERVE_CONFIG_BUFFER(resolved_path, PATH_MAX);
>> #endif
>>
>> - /* no options, no getopt */
>> -
>> if (optind + 1 != argc)
>> bb_show_usage();
>
>Why delete the comment?
Because it looked wrong. (what's the unconditional use of optind doing
there if getopt isn't involved?)
Apart from that that applet itself is bloated and needs to be touched
anyway.
>
>> @@ -58,9 +56,8 @@ int readlink_main(int argc, char **argv)
>> if (!buf)
>> return EXIT_FAILURE;
>> puts(buf);
>> -#ifdef CONFIG_FEATURE_CLEAN_UP
>> - free(buf);
>> -#endif
>> +
>> + if (ENABLE_FEATURE_CLEAN_UP) free(buf);
>
>This doesn't change anything relevant, except that people with older
>compilers or compilers for certain architectures will get the
>(pointless) free call despite having disabled this "feature" (some
>people may event want to *gasp* turn the optimizer off ...).
>
>It is still a bad idea to do this cleanup-stuff, which only ever
>makes sense for people who don't run busybox as part of a full-blown
>operating system, in each applet, instead of fixing this up *in* *one*
>*place* (namely, the exit routine).
I'm not happy to rely on dead-code elimination in general.
I'd have put all those into an EXIT_CONFIG_BUFFER macro or for my part
in an xfree() function. None of my business, though.
>
>And it is still an extraordinary stupid concept to litter source code
>with compiler-dependant stuff whose purpose is to make the code worse
>except if an optimizer supports certain features. It's not that it is
>getting bored if it doesn't have anything to do.
>
>Aber das ist natuerlich Pfeifen im Walde.
Wie auch immer.
More information about the busybox
mailing list