Ticket #467 (closed todo: fixed)

Opened 5 years ago

Last modified 4 years ago

find_method op should get sub pmc for pmclass method

Reported by: ronaldws Owned by: whiteknight
Priority: normal Milestone:
Component: core Version:
Severity: medium Keywords: ops, pmc
Cc: Language:
Patch status: Platform:

Description

This bug report recommends, based on an IRC discussion, that the find_method operation be able to retrieve a sub pmc for a method declared in a pmclass declaration.

I asked on IRC about getting a sub pmc for a pmc method declared in a pmclass declaration without looking for the method name in a namespace. It was suggested that I try the find_method op on a pmc instance and, when that didn't work, some of the discussion participants, who felt that it should, recommended a trac ticket.

I have also attached a patch that would add a relevant test to t/pmc/pmc.t.

Attachments

pmc.patch Download (1.5 KB) - added by ronaldws 5 years ago.
pmc.2.patch Download (1.6 KB) - added by ronaldws 4 years ago.
A revised, updated, patch of tests related to this ticket
object-meths.patch Download (1.4 KB) - added by ronaldws 4 years ago.
New revised test file with one more test for builtin

Change History

Changed 5 years ago by ronaldws

Changed 5 years ago by whiteknight

  • keywords ops, pmc added
  • status changed from new to assigned
  • component changed from none to core
  • type changed from bug to todo
  • owner set to whiteknight

Changed 4 years ago by jkeenan

Can we get an update on the status of this patch?

Thank you very much.
kid51

Changed 4 years ago by ronaldws

A revised, updated, patch of tests related to this ticket

Changed 4 years ago by ronaldws

When I apply the updated patch and run the tests, the patch shows that the fundamental issue raised in the ticket has been addressed at this time.

Looking at the original tests, it seems that, during the IRC discussion that started this trac ticket to begin with, a request was made to prevent pmc method subs from being retrievable through the class namespace. I vaguely recall that the issue was namespace pollution but am not sure and it may also have been about inheritance or other possible pmc instance transformations. I also don't really know whether it is still considered a concern.

Reviewing the IRC discussion for the link below, I don't see the reason for the restriction, but am a little reluctant to just drop it without another pair of eyes taking a look.

 http://irclog.perlgeek.de/parrot/2009-03-10#i_972463

Changed 4 years ago by ronaldws

The issue mentioned in my prior comment about pmc method subs being retrievable through the class namespace seems to be covered by trac ticket 389  http://trac.parrot.org/parrot/ticket/389. I am inclined to move the test from pmc.2.patch labeled _ns_test out, possibly to ticket 389, apply the remainder of the patch to pmc.t and request the closure of this ticket. Commentary, particularly by anyone familiar with TT 389 appreciated.

Thanks, Ron

Changed 4 years ago by ronaldws

New revised test file with one more test for builtin

Changed 4 years ago by ronaldws

I found an existing test for this functionality in t/pmc/object-meths.t coded in pasm. I could not figure out how long it had been there but after looking at the svn log found no reason to believe it had been added or modified recently. My initial test case used a String PMC object which was created from the builtin String PMC class which, it now appears, may have been the source of the problem with find_method in the first place, I'm not sure. In any case the latest attached patch translates the existing pasm test to pir and adds a test for the builtin pmc String class that opened the ticket.

I think it is pretty much time to close this ticket. I would be in favor of applying the attached object-meths.patch but I have been away from parrot development for a while and would like someone more closely involved to review my suggestions. Ticket 389, mentioned in my previous comment, is now closed.

Thanks, Ron

Changed 4 years ago by whiteknight

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

Applied with some modifications in 6c3990d. Resolved.

Note: See TracTickets for help on using tickets.