FAQ
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

Search Discussions

  • Steve Peters at Feb 22, 2007 at 2:03 pm

    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
    steve@fisharerojo.org
  • Jim Cromie at Mar 1, 2007 at 4:10 am

    Steve Peters wrote:
    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.
    heres an incremental patch to do the above.


    Its been a while since I did a -Dmad build (in Nov I think),
    IIRC it failed a few tests. I'll try one on this latest patchset soon.

    thanks
    jimc
  • Jim Cromie at Mar 13, 2007 at 8:13 am

    Steve Peters wrote:
    On Wed, Feb 21, 2007 at 10:35:53PM -0700, Jim Cromie wrote:

    +/* 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.
    hi Steve,

    heres the 4th patch again, with 1 less chunk, others as previously sent
    and a 5th patch, which:

    juggles embed.fnc entries, gathering those implemented in dump.c together
    - I wanted to look for naming patterns in the routines
    - the *_dump() have corresponding do_*_dump correllates, with different args
    - Im not sold that this is an improvement, or even a good idea

    adds this for the newly factored bits.

    #if defined(PERL_IN_DUMP_C)
    : this feels dirty - exposing static fns, but only in DUMP_C - why bother ?
    s |SV* |dump_op_flags |const OP* o
    s |SV* |dump_op_flags_private |const OP* o
    s |void |dump_op_mad |I32 level|NN PerlIO *file|NN const OP *o
    s |void |dump_op_rest |I32 level|NN PerlIO *file|NN const OP *o
    #endif

    naming is hard - as you can see, I punted on good ones.

    I must say, I don't quite see the benefit of doing this embed-ing
    The new functions are purely implementation details, not shared anywhere.
    For this purpose, pure C static declarations work, without needing
    make regen magic, which feels to me like an action at a distance -
    - make regen, embed.pl, dependencies etc..

    It strikes me that embed.fnc is about controlled sharing between objects,
    with conveniences like handling pTHX, making Perl_ prefix optional, etc.
    For non-sharing cases, we dont need all the extra mechanism,
    and could / should use plain vanilla static.
    But Im just thinking out loud.
  • Jim Cromie at Jun 12, 2007 at 12:46 am

    Steve Peters wrote:
    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.
    I guess my corollary of the dilemma (see wikipidia) is between :
    5 - No one cares about the post, for whatever reason.
    other - round tuits are too rare to use on boring/tedious patches
    cc - adding 1 cc lets everyone else off the hook (oops)
    - this is its own sub-dilemma


    The last resend was 5/11, Ive updated several since then, which Im
    attaching. The un-resent ones still work, maybe with fuzz,

    Heres the synopsis again (in order)

    diff.dump-test - adds debugging.t, only for -DEBUG builds
    diff.dump-refactor - chop long func into 1 + 2 helpers
    diff.dump-reduce - reuse helpers
    diff.dump-one-opmad - push ifdef down into function body
    diff.dump-reindent - cosmetic
    diff.dump-chgsig - chg helpers to share and mod single tmpsv
    diff.dump-embed - disagree, but not enough..

    blead + diff.dump-const - baseline under this series

    passes all tests when built with -Dmad and -DDEBUGGING,
    producing these sizes: (non-changes elided)

    [jimc@harpo dev]$ size dumpM-?/dump.o
    text data bss dec hex filename
    117932 0 0 117932 1ccac dumpM-0/dump.o
    118106 0 0 118106 1cd5a dumpM-2/dump.o
    114737 0 0 114737 1c031 dumpM-3/dump.o
    114277 0 0 114277 1be65 dumpM-6/dump.o

    ie a 3% obj shrink ( in an admittedly uncommon build, but hey, its about
    less code )

    [jimc@harpo dev]$ wc -l dumpI-?/dump.c
    2922 dumpI-0/dump.c
    2922 dumpI-1/dump.c
    2968 dumpI-2/dump.c
    2782 dumpI-3/dump.c
    2779 dumpI-4/dump.c
    2777 dumpI-5/dump.c
    2768 dumpI-6/dump.c
    2768 dumpI-7/dump.c


    have the appropriate amt of fun ;-)
    -jimc

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupperl5-porters @
categoriesperl
postedFeb 22, '07 at 5:35a
activeJun 12, '07 at 12:46a
posts5
users2
websiteperl.org

2 users in discussion

Jim Cromie: 4 posts Steve Peters: 1 post

People

Translate

site design / logo © 2021 Grokbase