svn commit: trunk/busybox/networking

[Bernhard Fischer]

On Sat, Jul 01, 2006 at 02:19:22PM -0400, Robert P. J. Day wrote:
>On Sat, 1 Jul 2006, Bernhard Fischer wrote:
>
>> On Sat, Jul 01, 2006 at 08:00:01AM -0700, rpjday at busybox.net wrote:
>> >Author: rpjday
>> >Date: 2006-07-01 07:59:54 -0700 (Sat, 01 Jul 2006)
>> >New Revision: 15572
>>
>> >Modified: trunk/busybox/networking/ifupdown.c
>> >===================================================================
>> >--- trunk/busybox/networking/ifupdown.c	2006-07-01 14:52:12 UTC (rev 15571)
>> >+++ trunk/busybox/networking/ifupdown.c	2006-07-01 14:59:54 UTC (rev 15572)
>> >@@ -43,11 +43,7 @@
>> > #define MAX_INTERFACE_LENGTH 10
>> > #endif
>> >
>> >-#if 0
>> >-#define debug_noise(fmt, args...) printf(fmt, ## args)
>> >-#else
>> > #define debug_noise(fmt, args...)
>> >-#endif
>> >
>> > /* Forward declaration */
>> > struct interface_defn_t;
>>
>> Whoever that may be..
>> Doing stuff like this is nonsense, to say the least.
>
>um ... no, it isn't, since the notion of removing "dead" or "unused"
>code in the source tree has been discussed more than once before, and
>no one seemed all that upset by the suggestion at the time.
>
>> You're breaking the conventient possibility to turn on debugging
>> facilities with what reasoning again?
>
>because that's an immensely silly way to include conditional debugging
>code.  if an individual maintainer wants to include that sort of
>thing, then they should at least do it using a meaningful directive,
>such as
>
>#ifdef DEBUG
>
>even better, each maintainer could use package-specific directives,
>like
>
>#ifdef HTTPD_DEBUG
>
>or something like that.  there's absolutely nothing wrong with
>maintainers wanting the ability to turn on debugging in their own
>packages, but using "#if 0" to deal with it is just plain silly since
>it requires manual editing, rather than being able to simply define
>the appropriate macro at build time.
>
>more to the point, using "#if 0" *anywhere* in the source tree is
>utterly uninformative.  when you see it, it may not be clear whether
>that's truly dead, obsolete code or questionable code or debugging
>code or code that's there for future consideration or code that people
>forgot about five years ago or what.
>
>now, in some cases, it's clear based on a comment, as in httpd.c:
>
>typedef enum
>{
>  HTTP_OK = 200,
>  HTTP_MOVED_TEMPORARILY = 302,
>  HTTP_BAD_REQUEST = 400,       /* malformed syntax */
>  HTTP_UNAUTHORIZED = 401, /* authentication needed, respond with auth hdr */
>  HTTP_NOT_FOUND = 404,
>  HTTP_FORBIDDEN = 403,
>  HTTP_REQUEST_TIMEOUT = 408,
>  HTTP_NOT_IMPLEMENTED = 501,   /* used for unrecognized requests */
>  HTTP_INTERNAL_SERVER_ERROR = 500,
>#if 0 /* future use */
>  HTTP_CONTINUE = 100,
>  HTTP_SWITCHING_PROTOCOLS = 101,
>  HTTP_CREATED = 201,
>  ...
>
>but it would *still* make more sense to use one of:
>
>#ifdef TODO
>#ifdef FUTURE_WORK
>#ifdef NOT_DONE_HERE_YET
>
>or something like that.  personally, i'd simply ban the use of "#if 0"
>outright and force people to choose their directives so that they're
>more meaningful.  there's simply no defense for either of "#if 0" or
>"#if 1" and, one way or another, they should all be either removed or
>rewritten to be more informative.

Did you read my remark (the one you didn't quote here and which boiled
down to parts of what you wrote above)?

Either remove it completely or export it to the config system, nothing
in between.
In the case of e.g. ifupdown.c you did remove it partially, which buys
absolutely nothing except removing an occurance of '#if 0'.

PS: also, please don't forget to update the boilerplates while you are
touching files, TIA.