Ticket #1988 (closed bug: fixed)

Opened 11 years ago

Last modified 11 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

Description

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:

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

SYNOPSIS
        use Parrot::Pmc2c::PMCEmitter;

DESCRIPTION
    "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.

kid51

Attachments

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

Change History

  Changed 11 years ago by jkeenan

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

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

config/auto/pmc.pm
65:    lib/Parrot/Pmc2c/PMCEmitter.pm \\

lib/Parrot/Pmc2c/Method.pm
206:    lib/Parrot/Pmc2c/PMCEmitter.pm

lib/Parrot/Config/Generated.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.

Suggestions?

  Changed 11 years ago by jimmy

Hello,

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

Thanks.

JimmyZ

  Changed 11 years ago by NotFound

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

  Changed 11 years ago by cotto

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

  Changed 11 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 11 years ago by jkeenan

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

Changed 11 years ago by jkeenan

Current diff between master and tt1988_pmcemitter branch

  Changed 11 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.

kid51

follow-up: ↓ 9   Changed 11 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.

kid51

in reply to: ↑ 8   Changed 11 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.

kid51

Note: See TracTickets for help on using tickets.