Evaluate all hack-identifying comments in source code

[jkeenan]

A couple of years back, Paul T Cochrane grepped our repository for 'TODO', 'FIXME' and 'XXX' and created RT tickets for all comments containing these strings. We've incorporated some of that in our test suite.

Tonight I realized that we have many comments in our source code which identify adjacent code as hacks. Some of these are already being tracked in RT or TT; others are not yet tracked.

Before creating a raft of new TTs, I'd like to post an edited list of what I got from grep-ing trunk for 'hack'. If you can respond to any of these this-is-a-hack comments, please do so.

Thank you very much.
kid51

$ cat ~/learn/parrot/hacks.edited.txt
./src/gc/resources.c:495:                        /* Somewhat of a hack, but if we get per-pool
./src/gc/mark_sweep.c:621:        /* XXX FIXME hack */
./src/gc/mark_sweep.c:736:    /* yes, this cast is a hack for now, but a pointer is a pointer */
./src/gc/mark_sweep.c:992:        /* short-term hack to color objects black */
./src/gc/system.c:78:            double align_hack[2];
./src/gc/system.c:91:        static void (*fn_ptr)(void) = (void (*)(void))&u.align_hack[0];
./src/string/api.c:147:        /* XXX FIXME hack to avoid cross-interpreter issue until it
./src/pmc/class.pmc:1586:This is just a temporary hack. Type ID numbers shouldn't be externally
./src/pmc/complex.pmc:537:Quick hack to emulate get_real() and get_imag():
./src/pmc/null.pmc:49:        /* XXX maybe a hack to get TGE running again */
./src/pmc/parrotinterpreter.pmc:390:        /* quick hack to get the global stash */
./src/pmc/parrotthread.pmc:46: * XXX a quick hack to pass the few tests
./src/pmc/fixedintegerarray.pmc:173:        /* a quick hack to create a clone in the constant PMC arena
./src/pmc/namespace.pmc:324:            /* TODO - this hack needs to go */
./src/pmc/nci.pmc:327:                /* Parrot_eprintf(interp, "HACKED %S\n", nci_info->signature); */
./src/pmc/default.pmc:88:    /* the quick hack below cannot be used because the string could
./src/pmc/default.pmc:92:     * a quick hack, to prevent freeing that string during GC
./src/jit/arm/jit_emit.h:490:   are on (ie MOV followed by anything, including hacks with setting the carry
./src/jit/ppc/jit_emit.h:1223:     * sets condition codes, so the speed hack in Parrot_ifunless_i_ic
./src/jit/sun4/jit_emit.h:19:   XXX Hack [perl #37819]  below.
./src/jit/sun4/jit_emit.h:797:/* XXX Hack to fix the following bug as of 05 Dec 2005:
./src/jit/sun4/jit_emit.h:798:    Hack [perl #37819] Sun4 builds fail linking against jit.o
./src/jit/sun4/jit_emit.h:802:    See also the "Hack [perl #37819]" section near the bottom of this
./src/jit/sun4/jit_emit.h:824:/* XXX end blind hack for [perl #37819] --but see
./src/jit/sun4/jit_emit.h:825:   additional hack section at bottom.
./src/jit/sun4/jit_emit.h:1180:/* XXX Hack [perl #37819]  Unlike the functions above, this one apparently
./src/jit/amd64/jit_emit.h:808: * to track down, the hackish things to do(movhlpd and movlhpd were used, but I
./src/io/unix.c:698:        /* XXX ugly hack to be able to pass some arguments
./src/oo.c:228:        /* This is a hack! All PMCs should be able to be handled through
./src/global.c:888:        /* TEMPORARY HACK - cache invalidation should be a namespace function */
./src/pic.c:99: * hack to turn on inlining - just sub_p_p for mops done
./src/list.c:854:         * Jr's book "Hacker's Delight". */
./src/malloc.c:2191:      S. Warren Jr's book "Hacker's Delight".
./src/malloc.c:2738:  probably figure out how to hack this routine to print out or
./src/thread.c:1128:    /* XXX: maybe a little hack; see RT #49532 */
./ext/SQLite3/DBDI/Driver/SQLite3.pm:12:    # This bit of hackery is to get around the fact that you can't use
./tools/build/nativecall.pl:332:   hackish, but that is just fine */
./tools/build/nativecall.pl:524:       "here there be hacks" */
./tools/util/pgegrep:140:       stdindashhack:
./tools/util/pgegrep:143:               goto stdinhack
./tools/util/pgegrep:148:                       goto stdindashhack
./tools/util/pgegrep:180:       stdinhack:
./tools/dev/parrot-fuzzer:470:        #This is a moderately fragile hack that relies on the specific
./lib/Parrot/Configure/Options/Conf.pm:83:   --define=inet_aton   Quick hack to use inet_aton instead of inet_pton
./lib/Parrot/Docs/POD2HTML.pm:248:            # A little bit of a hack to avoid config template files.
./lib/Parrot/Docs/File.pm:71:    'hacking'      => 'README file',
./include/parrot/caches.h:18:/* turn off this hack, we need something better */
./compilers/pct/src/PCT/Grammar.pir:188:    ##  the 'o', 'd', or 'x' into 8, 10, or 16 (yes, it's hack
./compilers/imcc/imclexer.c:2408: * we AFAIK need this hack as flex doesn't export YY_BUFFER_STATE */
./compilers/imcc/main.c:440:            case '.':  /* Give Windows Parrot hackers an opportunity to
./compilers/imcc/imcc.l:49: * we AFAIK need this hack as flex doesn't export YY_BUFFER_STATE */
./compilers/pge/PGE/P5Regex.pir:115:    optable.'newtok'('close:}', 'looser'=>'infix:|', 'nows'=>1)            # XXX: hack
./compilers/pirc/src/pir.l:405:{DQ_STRING}       { /* XXX this is a bit hacky. First the string is unescaped, but that
./compilers/pirc/src/pirlexer.c:2586:{ /* XXX this is a bit hacky. First the string is unescaped, but that
./t/pmc/signal.t:48:# This is a non-portable hack.
./t/pmc/class.t:89:    $I0 = 1        # hack for testing exceptions
./t/steps/auto_arch-01.t:44:    # Darwin because of a hack in config/auto/arch.pm.  We capture the warning
./t/op/sprintf_tests:201:%d     $p=sprintf('%p',$p);$p=~/^[0-9a-f]+$/   1       Coarse hack: hex from %p?
./t/op/sprintf_tests:202:%d     $p=sprintf('%-8p',$p);$p=~/^[0-9a-f]+\s*$/      1       Coarse hack: hex from %p?
./config/inter/make.pm:123:        # RT#43171 this is an ugly hack
./config/auto/alignptrs/test_c.in:43:        printf("Soft failure hack for systems that simulate unaligned access\n");
./config/auto/headers.pm:42:        # headers we found so far. This is somewhat a hack, but makes probing
./config/gen/opengl.pm:320:    # Ignore internal GLUT Win32 compatibility hackage
./config/gen/opengl.pm:649:                $ignore{$file}++, next if $name =~ /_ATEXIT_HACK$/;

[doughera]

Replying to jkeenan:

A couple of years back, Paul T Cochrane grepped our repository for 'TODO', 'FIXME' and 'XXX' and created RT tickets for all comments containing these strings. We've incorporated some of that in our test suite.

./src/gc/system.c:78:            double align_hack[2];
./src/gc/system.c:91:        static void (*fn_ptr)(void) = (void *)(void))&u.align_hack[0];

This section is already tracked in TT 271 (mentioned in the comments a few lines earlier in the file.)

> ./src/jit/sun4/jit_emit.h:19:   XXX Hack [perl #37819]  below.
> ./src/jit/sun4/jit_emit.h:797:/* XXX Hack to fix the following bug as of 05 Dec 2005:
> ./src/jit/sun4/jit_emit.h:798:    Hack [perl #37819] Sun4 builds fail linking against jit.o
> ./src/jit/sun4/jit_emit.h:802:    See also the "Hack [perl #37819]" section near the bottom of this
> ./src/jit/sun4/jit_emit.h:824:/* XXX end blind hack for [perl #37819] --but see
> ./src/jit/sun4/jit_emit.h:825:   additional hack section at bottom.
> ./src/jit/sun4/jit_emit.h:1180:/* XXX Hack [perl #37819]  Unlike the functions above, this one 
apparently

These are all tracked in the same RT #37819, which is unlikely to ever get fixed. JIT is currently disabled on the sun4 architecture.

./lib/Parrot/Configure/Options/Conf.pm:83: --define=inet_aton Quick hack to use inet_aton instead of inet_pton

Some years ago, I submitted a patch to handle this automatically, but it was rejected.

./config/inter/make.pm:123: # RT#43171 this is an ugly hack

The last few entries in the RT accurately summarize the current state of affairs for this.

./config/auto/alignptrs/test_c.in:43: printf("Soft failure hack for systems that simulate unaligned access\n");

The word "hack" is perhaps unfortunate here -- the test is doing the right thing in this case. (Though ultimately, it doesn't matter, since the results of the test aren't used anywhere.)

./config/auto/headers.pm:42: # headers we found so far. This is somewhat a hack, but makes probing

This is also doing the right thing already. It's not really a "hack".

[jkeenan]

Replying to doughera:

Thanks, Andy. That's exactly the sort of feedback I was looking for. It will enable us to exclude those instances from the creation of additional tickets.

kid51

[cotto]

I'd very strongly prefer to use a whitelist rather than a blacklist for deciding which suspected problems deserve tickets. The old rt tickets that refer to specific "XXX" or "TODO" and say something along the lines of "This needs to be fixed," are some of the least helpful tickets in that system. They generally give a vague idea that something might have been wrong at some point in Parrot's (long) history. Often they're so terse that a good deal of digging is necessary to even figure out if the problem still exists, and if the problem is worth fixing. This kind of work should be done *before* a ticket is filed so that the tickets in tt at least describe known problems.

Since it's unlikely that any of these suspected issue are currently causing a major problem*, I suggest a wiki page to track all the issues mentioned in kid51's list. The page could be used to weed out hacks that aren't worth fixing (i.e. those in code that will be replaced in the foreseeable future or code that's not worth fixing) so that we don't end up dumping more unhelpful tickets into tt.

* Ugly but functional code is not a major problem, though we're always glad to get rid of it.

[cotto]

I'd like to close this ticket. We do want to have a way to draw attention to noteworthy comments, so here's what I'd propose: We should find some enterprising gci student to write a script (perl, PIR or nqp-rx) in tools/dev to identify all comments, mangle it into trac wiki syntax and add some links to individual files and lines on github ( example). Once we have such a script, we can update the wiki with its output as needed and we can close this ticket.

[Yuki`N]

tools/dev/findhacks.pl slightly alleviates the problems addressed in this ticket. Although there is no 'whitelisting' functionality built-in, the script generates a tracwiki page which can be posted directly, identifying all 'hack' comments so that they can be reviewed and possibly deleted. In addition, this will help identify which points are worth fixing and also provides full context. A link is provided to the originating source file as well.

[whiteknight]

The output of this script is being displayed at ParrotHackList. I think we can close this ticket now.