develooper Front page | perl.perl5.porters | Postings from February 2007

Re: refactor dump.c

Thread Previous | Thread Next
Steve Peters
February 22, 2007 06:03
Re: refactor dump.c
Message ID:
On Wed, Feb 21, 2007 at 10:35:53PM -0700, Jim Cromie wrote:
> dump.c has a large swath of code in op_dump which has been cut-pasted,
> 756..956 and 2549..  with the former differing by the addition
> and subsequent use of an intermediate var;
>     const OPCODE optype = o->op_type;
> this 3 patch set refactors that code, then calls it
> - adds t/run/debugging.t - with 1 test of op-dump
> - refactor Perl_do_op_dump - somewhat ideosyncratic to lower diffsize
> - reuse refactoring in Perl_do_op_xmldump

I've got a few comments about the attached code.  First, I'll give the 
test a run in a bit, and if it seems OK, I may add it regardless of the 
other changes.

> +/* forward decls - minimize diff */
> +STATIC SV* S_dump_op_flags(pTHX_ const OP* o);
> +STATIC SV* S_dump_op_flags_private(pTHX_ const OP* o);
> +static void S_dump_op_mad(pTHX_ I32 level, PerlIO *file, const OP *o);
> +static void S_dump_op_rest(pTHX_ I32 level, PerlIO *file, const OP *o);
> +

I'd rather see these done in embed.fnc than left in here.

> +#ifndef PERL_MAD
> +static void S_dump_op_mad (pTHX_ I32 level, PerlIO *file, const OP *o)
> +{
> +    PERL_UNUSED_ARG(level);
> +    PERL_UNUSED_ARG(file);
> +    PERL_UNUSED_ARG(o);
> +}
> +#else
> +static void S_dump_op_mad (pTHX_ I32 level, PerlIO *file, const OP *o)
> +{
>      if (PL_madskills && o->op_madprop) {
>  	SV * const tmpsv = newSVpvn("", 0);
>  	MADPROP* mp = o->op_madprop;

For this, I'd really prefer to see one function declaration rather than
two.  I think instead that the code should be #ifndef'd inside the functions
rather than #ifndef'ing the whole thing.  I have two reasons for it.  First,
its less code.  Second, its less error prone.  Someone will change one but 
not the other.

Otherwise, I really like this idea and I hope with these tweaks it will
help dump quite a bit of code out of dump.c.

Steve Peters

Thread Previous | Thread Next Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About