Ticket #103 (closed experimental: fixed)

Opened 6 years ago

Last modified 4 years ago

No self in PIR invoke VTABLE

Reported by: whiteknight Owned by: whiteknight
Priority: normal Milestone:
Component: imcc Version: trunk
Severity: medium Keywords:
Cc: Language:
Patch status: new Platform: all

Description

This issue was discussed on the list, and in some comments from RT#47674.

When overriding the invoke VTABLE interface in PIR, the invocant object is not passed as "self" to the method. This means that from the invoke vtable method we can't access the invoked object, any of it's attributes, or invoke the actual Sub PMC.

This needs to be fixed (and probably explained better).

Attachments

self.patch Download (53.4 KB) - added by kjs 5 years ago.
self now added when :vtable set.
invokeselfpatch.patch Download (439 bytes) - added by whiteknight 5 years ago.
add an invoke object as "self" in IMCC

Change History

  Changed 5 years ago by kjs

  • priority changed from normal to blocker
  • component changed from none to imcc
  • type changed from bug to todo

Changed 5 years ago by kjs

self now added when :vtable set.

  Changed 5 years ago by kjs

  • patch set to new

Before applying the attached patch, this didn't work: (indicating that it expects only 1 parameter, not 2) After applying the patch, it prints "1" on my system.

.sub main
  $P1 = get_class 'String'
  $P2 = subclass $P1, 'barf'
  $P3 = new 'barf'
  $I0 = does $P3, 'frobulate'
  say $I0
.end

.namespace ['barf']

.sub 'does' :vtable
  .param string what
  .return(42)
.end

  Changed 5 years ago by chromatic

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

Applied in r36073, with the test code added to t/oo/vtableoverride.t.

follow-up: ↓ 8   Changed 5 years ago by whiteknight

  • status changed from closed to reopened
  • resolution fixed deleted

This patch does fix one related issue, but doesn't fix the exact issue raised here by this ticket. The "invoke" vtable method still does not have access to "self". All the other vtable methods seem to have this correctly, but invoke is a special case that still does not work correctly.

I'll try to add a TODO test today to t/oo/vtableoverride.t that demonstrates the problem.

  Changed 5 years ago by whiteknight

  • status changed from reopened to new
  • owner set to whiteknight

Changed 5 years ago by whiteknight

add an invoke object as "self" in IMCC

  Changed 5 years ago by whiteknight

  • status changed from new to assigned
  • priority changed from blocker to major

Okay, I've added a patch to this that *I think* might do something like what I am after here. I don't have Flex/Bison on this computer, so I can't test this till tonight. Hopefully it does what I want and does not do what I don't want.

  Changed 5 years ago by whiteknight

Just checked, the attached patch does not work. It breaks the build. I have a few more things I might try tonight.

in reply to: ↑ 4   Changed 5 years ago by kjs

Replying to whiteknight:

This patch does fix one related issue, but doesn't fix the exact issue raised here by this ticket. The "invoke" vtable method still does not have access to "self". All the other vtable methods seem to have this correctly, but invoke is a special case that still does not work correctly. I'll try to add a TODO test today to t/oo/vtableoverride.t that demonstrates the problem.

Can you indicate with an example in this ticket what's wrong? The following code does have access to "self":

.namespace ["Foo"]
.sub invoke :vtable
  .param pmc a
  say self  # prints "Hi I'm a Foo!"
  say 'hi'
.end

.sub get_string :vtable
    .return ("Hi I'm a Foo!")
.end

.namespace []
.sub main :main
  $P0 = newclass "Foo"
  $P1 = new "Foo"
  $P1($P1,1)
.end

This was the problem right? It prints on my system:

C:\cygwin\home\Kramer\parrot2>parrot self.pir
Hi I'm a Foo!
hi

  Changed 5 years ago by whiteknight

Yes, that does work. However you have to do the ugly syntax $P1($P1) to make it work. Calling $P1() without passing itself as the first parameter causes an error, and it shouldn't.

When we try to invoke a PMC, IMCC should cause that pmc to be passed as the first parameter automatically. The programmer should not have to pass it explicitly. This is just a workaround that should be temporary only.

  Changed 5 years ago by NotFound

I tested this trick that allows to have some sort of 'self' in the invoke override.

This type of tricks can allow to access the invocant, but in order to make the desired syntax to work, the call to invoke must not have the usual self passed.

I'll prefer to be able to use that tricks rather than have an invoke that does not allow to create usable functors.

.sub main
  $P0 = subclass 'Integer', 'Test'
  $P1 = new $P0
  $P1.'init'()
  $P1.'hello'()
  $P1 = 10
  say $P1
  null $P2
  $P1($P2)
.end

.namespace [ 'Test' ]

.sub 'hello' :method
  say 'Hello'
.end

.sub getself
  .param pmc myself :optional
again:
  .yield(myself)
  goto again
.end

.sub init :method
  'getself'(self)
.end

.sub 'invoke' :vtable
  .local pmc this
  this = 'getself'()
  this.'hello'()
  say this
.end

  Changed 5 years ago by whiteknight

  • milestone 1.0 deleted

  Changed 5 years ago by NotFound

Experimental feature added in r42424, see TT #1262

  Changed 4 years ago by coke

  • type changed from todo to experimental

  Changed 4 years ago by jkeenan

  • priority changed from major to normal

Is this ticket closable?

Thank you very much.

kid51

  Changed 4 years ago by whiteknight

It's not closable really until we remove the experimental marker for this issue. I haven't heard any complaints about it, but I am not sure if it is well tested. If there are passing tests in place and there are no complaints, I would be happy to close this ticket.

  Changed 4 years ago by NotFound

I'm using it. My examples of opengl usage from winxed depend on it. My tests with blizkost depend on it. Enough tests for the functionality I want.

Other possible functionalities, like the invokable being used as method on some other object and needing access both to self and the invoking object, I think they must be the subject of new RFC tickets, and thus this one can be closed.

  Changed 4 years ago by whiteknight

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

Okay, with that note we can close this ticket. If problems are found in the current implementation of this, we can open new tickets to address those problems.

We should discuss whether to stop marking this feature as "experimental", which I suggest we do since it's tested and is being used by other projects.

Note: See TracTickets for help on using tickets.