Ticket #1988 (closed bug: fixed)

Opened 6 years ago

Last modified 6 years ago

lib/Parrot/Pmc2c/PMCEmitter.pm: make file match package.

Reported by: jkeenan Owned by: jkeenan
Priority: normal Milestone:
Component: build Version: 3.0.0
Severity: medium Keywords:
Cc: tewk, mikehh Language:
Patch status: applied Platform: all


During cage-cleaning today I reviewed TT #682 in which chromatic commented on some consequences of a modification of lib/Parrot/Pmc2c/PMCEmitter.pm. I subsequently examined this file and was puzzled by what I saw:

1. The file is named lib/Parrot/Pmc2c/PMCEmitter.pm and the POD that appears at the head of the file is consistent with that:

    Parrot::Pmc2c::PMCEmitter - PMC to C Code Generation

        use Parrot::Pmc2c::PMCEmitter;

    "Parrot::Pmc2c::PMCEmitter" is used by tools/build/pmc2c.pl to generate
    C code from PMC files.

But then the package declaration is this:

package Parrot::Pmc2c::PMC;

... which flies in the face of this file: lib/Parrot/Pmc2c/PMC.pm, which itself contains a package Parrot::Pmc2c::PMC; declaration.

ack-ing for PMCEmitter shows this:

lib/Parrot/Pmc2c/Library.pm:27:use Parrot::Pmc2c::PMCEmitter ();
lib/Parrot/Pmc2c/PMC/RO.pm:28:use Parrot::Pmc2c::PMCEmitter ();

... but, AFAICT, no functions from lib/Parrot/Pmc2c/PMCEmitter.pm are ever called in Parrot::Pmc2c::Library or Parrot::Pmc2c::PMC::RO.

All of which suggests that this file is dead code which can be ripped out. I am going to start a branch to explore that possibility -- but does anyone have any better insight into this?

Thank you very much.



master.tt1988_pmcemitter.diff Download (68.9 KB) - added by jkeenan 6 years ago.
Current diff between master and tt1988_pmcemitter branch

Change History

  Changed 6 years ago by jkeenan

Hmm, I see that my search was not sufficiently broad:

$ ack 'PMCEmitter.pm' *
1874:    lib/Parrot/Pmc2c/PMCEmitter.pm \

65:    lib/Parrot/Pmc2c/PMCEmitter.pm \\

206:    lib/Parrot/Pmc2c/PMCEmitter.pm

502:    lib/Parrot/Pmc2c/PMCEmitter.pm \\

... which led to this build failure:

make: *** No rule to make target `lib/Parrot/Pmc2c/PMCEmitter.pm', needed by `src/pmc/default.dump'.  Stop.

Still, the notion that a file named lib/Parrot/X.pm does not contain a package called Parrot::X sticks in my throat. It's legal Perl 5, but it's certainly not a best practice -- particularly when a package of the same name appears in a different, correctly named file.


  Changed 6 years ago by jimmy


I'm not sure, but it's maintained. see  https://github.com/parrot/parrot/commits/master/lib/Parrot/Pmc2c/PMCEmitter.pm for git log.



  Changed 6 years ago by NotFound

The file is used. The changes in 2dcf61ee076cbd800a7e worked as expected.

  Changed 6 years ago by cotto

The file is definitely used, but +1 to make the file names match the package declarations.

  Changed 6 years ago by coke

  • summary changed from lib/Parrot/Pmc2c/PMCEmitter.pm: Is this file useless? to lib/Parrot/Pmc2c/PMCEmitter.pm: make file match package.

  Changed 6 years ago by jkeenan

  • owner set to jkeenan
  • cc tewk removed
  • status changed from new to assigned

Changed 6 years ago by jkeenan

Current diff between master and tt1988_pmcemitter branch

  Changed 6 years ago by jkeenan

  • cc tewk, mikehh added
  • platform set to all
  • component changed from configure to build
  • patch set to new

Much to my surprise, I was able to pick up all the methods in the PMCEmitter.pm's version of Parrot::Pmc2c::PMC, plunk them down in lib/Parrot/Pmc2c/PMC.pm and have everything just work! So I was able to eliminate one 1100+-lines-of-code file, albeit by moving the code elsewhere.

I was further surprised to learn that two entire packages, lib/Parrot/Pmc2c/PMC/ParrotClass.pm and lib/Parrot/Pmc2c/ComposedMethod.pm, were never used in the Parrot build process. We've been carrying these two files in our distribution for four years for no evident purpose at all. Two more files removed.

I have attached a patch showing the current differences between master and the tt1988_pmcemitter branch; please review.

I will be working on additional cleanup of code underneath the Parrot::Pmc2c::* hierarchy, but we should consider whether we want to try to merge this branch into master before or after the 3.1 release on February 15.

Thank you very much.


follow-up: ↓ 9   Changed 6 years ago by jkeenan

  • patch changed from new to applied

1. mikehh approved merge into master.

2. merged in commit 4c0e404bcb

3. bacek++ pointed out on IRC that we have the same problem with Method.pm and MethodEmitter.pm. I may not have time before the Feb 15 release to address that, so, having merged the branch into trunk, let's create a separate Trac ticket to address that problem: TT #2000.

4. Will keep this ticket open for a few days for comments or complaints.

Thank you very much.


in reply to: ↑ 8   Changed 6 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

Replying to jkeenan:

4. Will keep this ticket open for a few days for comments or complaints.

No complaints received; closing ticket.


Note: See TracTickets for help on using tickets.