Ticket #1988 (closed bug: fixed)

Opened 4 years ago

Last modified 4 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 4 years ago.
Current diff between master and tt1988_pmcemitter branch

Change History

  Changed 4 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 4 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 4 years ago by NotFound

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

  Changed 4 years ago by cotto

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

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

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

Changed 4 years ago by jkeenan

Current diff between master and tt1988_pmcemitter branch

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