svn commit: trunk/busybox/util-linux

Rob Landley rob at landley.net
Wed Sep 20 17:25:53 UTC 2006


On Wednesday 20 September 2006 10:05 am, Denis Vlasenko wrote:
> > > What will happen if somebody, somewhere in the jungle of included
> > > header files, already used "linenumber" as a name for something?
> > 
> > In an applet?  The new #define stomps the earlier one for any lines 
occurring 
> > later in the program.
> 
> No, not in applet. "linenumber" can be declared in a library header
> (libc or any other external library a programs compiles against)
> as a global variable. Just like FILE *stdout.

You're replacing a #declare with an enum, at the global level, in an applet.  
I see no reason to do this.

> Yes, it's debuggable. But it should be trivially debuggable,
> the same way when you accidentally declare
> 
> int syslog; // collides with function declaration

#define syslog 0
int syslog;

The compiler will let you know you screwed up there.

Also, there's a reason that you use ALL_CAPS when you #define stuff.  This is 
to prevent these sort of collisions.  It's a good thing, and the ALL_CAPS 
lets you know it's a #define rather than a variable.  Your enum looks just 
like a variable, despite being more like a #define.

Also, I'm used to python.  I'm used to finding things at runtime via a 
regression test harness.  "It compiled" gives me absolutely zero warm 
fuzzies, because there's no guarantee it'll work.  The syntax checker is just 
the first line of defense, and relying on the syntax checker more strikes me 
as a dubious activity at best.

> > > If putchar will expand to ....; something = linenuber;....,
> > > you get NO ERROR. You get nasty bug.
> > 
> > You get a behavior change, and you track it down.  I've tracked down much 
> > worse problems than this.
> 
> What if it is on error path? What if it triggers once in a year?

Regression test harness.  Test the error path in testsuite.

> > > In other words: C scope rules and typechecking are a Good Thing.
> > > We should use them, because they are designed to save us from
> > > common mistakes.
> > 
> > Are you saying that the above putchar() macro would still work when you 
made 
> > linenumber an enum, or are you just saying it would be easier to track 
down 
> > the bug?
> 
> With enum it will fail to compile right away at enum site.

With the ALL_CAPS name of defines, you don't have collisions in the first 
place, _and_ it's clear that you're using a constant rather than a variable.

> > (If linenumber is an enum and you declared it at a global level,  
> > and your bog is "something = linenumber" which is _in_ your current scope, 
> > how exactly is the enum an improvement?
> 
> putchar() is a macro, so "something = linenumber" is in new
> #define's scope if I use putchar() below #define line.
> So gcc happily converts it into "something = 0"

Where did "linenumber" come from in the first place?  In the macro you're 
trying to use.  If you declare it as a local variable, and you've #defined it 
to 0, it throws an error.  If it's one of the macro arguments, then that 
takes precedence in the macro scope over external redefinitions.

#include <stdio.h>

#define thingy(one,two) printf(one,two);
#define two walrus

int main(int argc, char *argv)
{
  thingy("hello %s\n","world");
}

gcc hello.c
./a.out
hello world

Can you come up with a real example of this causing an actual problem?  Give 
me something I can compile, like the above.  (P.S.  Use ALL_CAPS for your 
#define constants, as we do in the real world.)

> > And where did linenumber get declared in putchar, anyway?
> > Wouldn't "int 0;" cause the compiler to throw up?
> 
> It was a global variable declared and parsed somewhere up there
> in #include <something.h> from libc, way before gcc had a chance
> to see "#define linenumber 0"

Which is why your #define should have been ALL_CAPS.

> I see that a lot of you folks are really attached to #defines,

Yes, inlcuding me!

> so I wouldn't replace them by enums en masse. However I still think
> that they are unsafe. I will use enums in new code if you
> don't mind, ok?

I'm kind of attached to mount.c.  I rewrote the sucker three times.  I suspect 
your new changes have broken it.  (Did you run the regression test script?  
Which isn't close to complete, but it's a start.)

If you just want me to keep my hands off of mount.c in perpetuity and forward 
all maintenance requests to it to you, perhaps I can do that.  Except that 
when I did this:

svn update 16000
make distclean
make allnoconfig
make menuconfig: switch on mount, mtab support, an loopback.  (Leave nfs off.)
make baseline
svn update
make oldconfig  # for NFS_CIFS, which is switched off by default.
make

I got:

function                                             old     new   delta
bb_getopt_ulflags                                      -     884    +884
append_mount_options                                  55     183    +128
bb_verror_msg                                         56     130     +74
mount_it_now                                         277     330     +53
llist_add_to                                           -      28     +28
bb_perror_msg_and_die                                 29      47     +18
bb_error_msg_and_die                                  42      50      +8
msg_eol                                                -       4      +4
logmode                                                -       4      +4
die_sleep                                              -       4      +4
bb_opt_complementally                                  -       4      +4
bb_applet_long_options                                 -       4      +4
xstrdup                                               33      32      -1
xrealloc                                              38      37      -1
xmalloc                                               33      32      -1
busybox_main                                         196     191      -5
singlemount                                          633     626      -7
bb_path_mtab_file                                     10       -     -10
bb_error_msg                                          35      24     -11
mount_main                                           970     931     -39
bb_vperror_msg                                        74      30     -44
.rodata                                             1104    1053     -51
------------------------------------------------------------------------------
(add/remove: 7/1 grow/shrink: 5/9 up/down: 1213/-170)        Total: 1043 bytes

This does not fill me with warm fuzzies.

> And Rob, regarding that ssh problem - it's something on ISP
> side, not on busybox.net... I will find out what's going on.

Ouch.

Rob
-- 
Never bet against the cheap plastic solution.



More information about the busybox mailing list