[quagga-dev 3111] Re: list loop cleanup/audit
Andrew J. Schorr
aschorr at telemetry-investments.com
Tue Apr 5 15:13:33 BST 2005
On Tue, Apr 05, 2005 at 11:58:35AM +0100, Paul Jakma wrote:
> The old macros are preserved inside a 'QUAGGA_DEPRECATED' define,
> which should be set to a date. This possibly could be done a wee bit
> The full diff, large, removing LIST_LOOP and auditing just about
> every list loop in the tree is at:
I have a few comments, perhaps repetitive of previous comments, sorry
about that :-) :
1. Is the assert in listgetdata really needed? I would guess that in
most cases a SEGV would occur moments later if the pointer is actually
NULL. And if there is a SEGV, we should get a log message indicating
the crash with a backtrace. That is pretty much the same result as an
assertion failure, although not quite as user-friendly...
2. It seems that you started using the new macros in hundreds of spots (I
count 699 occurrences of ALL_LIST_ELEMENTS in the diff), some of which
may be bug fixes. But others may be changes to working non-buggy code.
This leads to the issue that Hasso has raised: does it make sense to change
non-buggy code, thereby decreasing source code compatibility with zebra?
I have no strong opinion on this, but we should come to a consensus.
I presume that the 325 instances of ALL_LIST_ELEMENTS_RO are not actually
fixing bugs; is that safe to say?
3. Regarding the QUAGGA_DEPRECATED stuff: isn't the net result going to
be a warning message every time a source file is compiled? It would
be quite useful if the message only occurred when a deprecated macro
is actually used, but it's not clear to me that it's so useful to
get the message for every single compilation. Is this just a club to
enforce removing those old macros eventually? And this ties in with
issue #2: is that really desirable?
Certainly, if we were starting from scratch, I would vastly prefer your
macros to the existing ones, but does it make sense to convert everything
at this point? Or just to convert as needed to fix bugs?
More information about the Quagga-dev