[GH #372] Remove morph VTABLE #955

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants
@paultcochrane
Contributor

paultcochrane commented Apr 10, 2013

These changes remove the morph VTABLE. I'm not 100% sure that I've done everything here correctly (especially the code in check_set_std_props; marked in the function), however the tests pass and so this doesn't break anything major. As usual, comments welcome.

paultcochrane added some commits Apr 9, 2013

[GH #372] Removed the morph op
This is necessary so that we can remove the morph VTABLE as mentioned in
GH #372
[GH #372] Corrected tests using the 'morph' op
Now the test suite passes again :-)
[GH #372] Replaced PMCNULL-setting morphs with the code directly
In src/call/args.c and src/embed/api.c VTABLE_morph() was just setting the
called object to PMCNULL.  The VTABLE_morph() has been replaced with a macro
MORPH_REPLACE_NULL which basically replicates the code in-place.  This
change should allow us to eventually remove the morph VTABLE entry.
[GH #372] Replaced VTABLE_morph() in check_set_std_props() with direc…
…t assignment

Hopefully this does what was intended (this comment is also part of this
change).  The tests pass, so at least the probability that nothing is broken
is high, but unfortunately not 100%.
@leto

This comment has been minimized.

Show comment
Hide comment
@leto

leto Apr 12, 2013

Member

You have committed some generated files. Some definitely shouldn't be added to the commit, others I am not so sure. Perhaps @cotto , @Benabik or @jkeenan could help with this?

Member

leto commented Apr 12, 2013

You have committed some generated files. Some definitely shouldn't be added to the commit, others I am not so sure. Perhaps @cotto , @Benabik or @jkeenan could help with this?

@leto

This comment has been minimized.

Show comment
Hide comment
@leto

leto Apr 12, 2013

Member

And thanks for this pull request!

Member

leto commented Apr 12, 2013

And thanks for this pull request!

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Apr 13, 2013

Contributor

I know I committed automatically generated files (from the ops change), however they are part of the change. Once upon a time there were automatically generated files which needed to be committed because they weren't built as part of the standard build process and as such would break things for normal users if they weren't committed. That the files I added here as part of this change are built as part of the standard perl Configure.pl begs the question as to whether they should be on the repo. What is the current feeling? Should these files be committed?

Contributor

paultcochrane commented Apr 13, 2013

I know I committed automatically generated files (from the ops change), however they are part of the change. Once upon a time there were automatically generated files which needed to be committed because they weren't built as part of the standard build process and as such would break things for normal users if they weren't committed. That the files I added here as part of this change are built as part of the standard perl Configure.pl begs the question as to whether they should be on the repo. What is the current feeling? Should these files be committed?

@leto

This comment has been minimized.

Show comment
Hide comment
@leto

leto Apr 13, 2013

Member

@paultcochrane the reason I asked is that sometimes generated code must be committed to break dependency cycles, but it is not clear to me if all the generated code in this PR should be committed. Perhaps @bacek the magic coding robot can bless us with his subtle wisdom?

Can we please document WHEN it is ok to commit generated code in our Git workflow document or one of the other internal developer docs?

Member

leto commented Apr 13, 2013

@paultcochrane the reason I asked is that sometimes generated code must be committed to break dependency cycles, but it is not clear to me if all the generated code in this PR should be committed. Perhaps @bacek the magic coding robot can bless us with his subtle wisdom?

Can we please document WHEN it is ok to commit generated code in our Git workflow document or one of the other internal developer docs?

@leto

This comment has been minimized.

Show comment
Hide comment
@leto

leto Jun 23, 2013

Member

I would like to request another code review from @cotto, @allisonrandal , @chromatic , @Whiteknight , @NotFound , @Benabik or someone else who knows about these things.

I want to merge this, but I am not sure everything in this PR is implemented correctly. Any guidance is greatly appreciated.

Member

leto commented Jun 23, 2013

I would like to request another code review from @cotto, @allisonrandal , @chromatic , @Whiteknight , @NotFound , @Benabik or someone else who knows about these things.

I want to merge this, but I am not sure everything in this PR is implemented correctly. Any guidance is greatly appreciated.

@rurban rurban self-assigned this Mar 5, 2014

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Mar 5, 2014

Member

PBC_COMPAT needs a bump and the native pbc's need to be regenerated. (done)

I didn't check the languages which used morph, to know how they can replace it with check_set_std_props() and MORPH_REPLACE_NULL()
I'd like to have a pir-level test which tests that new morphing without the morph op. (todo)

I don't like the unnecessary ops renumbering, pbc initially was thought to be backwards compat
from 1.0 on, esp. for languages, so the 2 ops should just be replaced by deprecation stubs. (left stubs)

Member

rurban commented Mar 5, 2014

PBC_COMPAT needs a bump and the native pbc's need to be regenerated. (done)

I didn't check the languages which used morph, to know how they can replace it with check_set_std_props() and MORPH_REPLACE_NULL()
I'd like to have a pir-level test which tests that new morphing without the morph op. (todo)

I don't like the unnecessary ops renumbering, pbc initially was thought to be backwards compat
from 1.0 on, esp. for languages, so the 2 ops should just be replaced by deprecation stubs. (left stubs)

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Mar 5, 2014

Member

pushed with some changes to branch ptc/rm_morph-gh37 but it is not ready yet

testcase missing to cover the new codepath in check_set_std_props:
src/pmc.c: pmc->vtable = (VTABLE*)interp->vtables[pmc->vtable->base_type - 1]->pmc_class;
The testcase should involve setprop of a _ro key to a false value with const vtable. Currently only undef and unmanagedstruct do a setprop

See GH #372

Member

rurban commented Mar 5, 2014

pushed with some changes to branch ptc/rm_morph-gh37 but it is not ready yet

testcase missing to cover the new codepath in check_set_std_props:
src/pmc.c: pmc->vtable = (VTABLE*)interp->vtables[pmc->vtable->base_type - 1]->pmc_class;
The testcase should involve setprop of a _ro key to a false value with const vtable. Currently only undef and unmanagedstruct do a setprop

See GH #372

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Mar 7, 2014

Member

I'm not so sure anymore if this is a good idea. parrot itself might not care about the morph vtable, but user objects need to define their custom morphing behavior. So I would stub our morph ops, but leave the vtable so that user pmcs can override it properly.

Member

rurban commented Mar 7, 2014

I'm not so sure anymore if this is a good idea. parrot itself might not care about the morph vtable, but user objects need to define their custom morphing behavior. So I would stub our morph ops, but leave the vtable so that user pmcs can override it properly.

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Mar 9, 2014

Member

I've decided we need to keep the morph vtable methods, esp. in the light of set_pmc casting/coercion GH #1026
But we also need to fix the default behavior, GH #372

Member

rurban commented Mar 9, 2014

I've decided we need to keep the morph vtable methods, esp. in the light of set_pmc casting/coercion GH #1026
But we also need to fix the default behavior, GH #372

@rurban rurban closed this Mar 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment