Re: [PATCH] Yet another tree dumper

[Gabriel Dos Reis <gdr at integrable-solutions dot net>]

Per Bothner <per@bothner.com> writes:

| Gabriel Dos Reis wrote:
| 
| > Per Bothner <per@bothner.com> writes:
| > | A suggestion:
| > | | Check in the new tree-dumper, but into the print-tree.c
| > | like this:
| > | | #define USE_NEW_DUMPER 1
| > | #ifdef NEW_DUMPER
| > we've gone a long way in moving toward
| >   if (SOME_FEATURE)
| >     // ..
| > instead of
| > #ifdef SOME_FEATURE.
| 
| #ifdef is better in this case.

I'm unconvinced.

| (1) it's intended to be temporary.  We're not testing a
| feature in the usual sense; we're selecting between two
| alternative implementations of the same utility.

Then, it is even better to say if(USE_NEW_DUMPER).

| (2) if (SOME_FEATURE) cannot conditionalize
| function definitions, as probably needed in this case.

That is not my impression after reading the patch -- remember it was
initialy intended sit next the existing one. 

Anyway, my take is that the new dumper should not sit or #ifdefed: it
should take parameters into account.  That concern was already
expressed and it was not addressed.

-- Gaby