Use internal function rather than inline PHP compile for automatic accessors #5

Closed
nikic opened this Issue Dec 29, 2012 · 16 comments

Projects

None yet

2 participants

@nikic
Collaborator
nikic commented Dec 29, 2012

Automatic accessors currently generate a piece of PHP code and compile it to generate the opcodes for the accessor. It would be cleaner and faster if a generic internal function would be used for this.

@nikic
Collaborator
nikic commented Dec 29, 2012

After some further consideration, I think this problem will (to a large part) solve itself. Once the shadowing behavior is inverted the automatic properties will be able to use the underlying property of the same name. Thus an automatic accessor is basically equivalent to just a normal property. So in this case we wouldn't even need an internal function, we could just use the normal read/write behavior if the automatic getter/setter is used.

The only thing that's not solved by this is the automatic isset/unset accessors. Those will not have the behavior, so to them this issue still applies.

@cpriest
Owner
cpriest commented Dec 29, 2012

Actually, the automatic getter/setter will be more difficult I think because there is no current way to distinguish between "not defined" and "automatically defined" since the getter is a pointer to a function. When it's automatically implemented, the executing code isn't aware of this at all, it's just a function to call.

If we don't have the code there, then we will need some other flag/mechanism to indicate that "it has, or has not" been defined, unless we wanted to muck with things like setting the function pointer to -1 or 0 or a standard function...

Not sure if that type of thing is frowned upon or not, I would think that -1 is highly unlikely to ever be a legally assigned memory address (or 1 or 2 ) or whatever.

@cpriest
Owner
cpriest commented Dec 29, 2012

For isset/unset that's a matter of just getting the property and checking it's null state or setting it's null state, that should be a snap.

@cpriest
Owner
cpriest commented Dec 29, 2012
class TimePeriod {
    private $Seconds;

    public $Hours {
        get();
        set([TypeHint] $value);
        isset();
        unset();
    }
}

/* Would automatically be implemented as */

class TimePeriod {
    private $Seconds;

    public $Hours {
        get() { return $this->Hours; }
        set([TypeHint] $value) { $this->Hours = $value; }
        isset() { return $this->Hours !== NULL; }
        unset() { $this->Hours = NULL; }
    }
}
@nikic
Collaborator
nikic commented Jan 2, 2013

For the in-handler implementation of these we would need to save the typehint separately and also handle it separately (as we can't rely any longer on the automatic stuff from the methods).

@cpriest
Owner
cpriest commented Jan 2, 2013

Hmm, isn't there a zend_internal_function structure I've seen somewhere?

@nikic
Collaborator
nikic commented Jan 2, 2013

@cpriest Yes there is. But I thought we decided to not go with internal functions here and rather use the -1 flag?

@cpriest
Owner
cpriest commented Jan 2, 2013

Yeah, we talked about that possibility but if that complicates things and there is a "standard" way to do it we probably should use the standard way, would that let us have type hints?

@nikic
Collaborator
nikic commented Jan 2, 2013

@cpriest We can have the type hints in any case with a bit of additional effort (i.e. create an argument_info for the typehint and store it in the ai structure). Using proper zend_functions for this would have other problems, e.g. one would need to somehow provide the property name to them. So I still think we should go with the implementation directly in the handler. Just wanted to point out that we need to look out for that ;)

@cpriest
Owner
cpriest commented Jan 3, 2013

This all seems like a lot of additional complexity just to avoid generated functions, no?

@nikic
Collaborator
nikic commented Jan 5, 2013

Just noticed that typehinted set without body currently doesn't work at all. public $foo { set(Bar $foo); } throws a parser error.

@cpriest
Owner
cpriest commented Jan 5, 2013

hmm, ok, I created a ticket for it: #31

@cpriest
Owner
cpriest commented Jan 5, 2013

Are you pursuing this yet? I think unless someone is against the current implementation we have bigger fish to fry...

@nikic
Collaborator
nikic commented Jan 12, 2013

Not pursuing this right now, would probably be unnecessarily complicated. I'll asks Dmitry whether he's okay with the generated code and if he is then we should leave it.

@cpriest
Owner
cpriest commented Jan 12, 2013

Agreed. When I did it I did some performance testing and I'm pretty certain I found no problems and thought it would be a bit "future proof" compared to my previous iteration, because that was basically calling all the zend_compile.c functions that would have been called had that been the code. That was fun to figure out. I figured this way, if the compiler changes, then this boilerplate auto-implemented code just gets updated w/ it.

@cpriest
Owner
cpriest commented Jan 13, 2013

Have you heard from Dmitry?

@cpriest cpriest closed this Jan 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment