Skip to content

Conversation

@Hywan
Copy link
Member

@Hywan Hywan commented Nov 18, 2014

Fix #16.

Tests are missing.

@Hywan Hywan force-pushed the stacked_contextes branch from 6725d74 to 70b6ce5 Compare November 18, 2014 08:37
@Hywan
Copy link
Member Author

Hywan commented Nov 18, 2014

Documentation is missing too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial pushContext would only make sense, if a popContext would also be triggered "automatically". but this seems to be not possible, so I would assume the caller should call both explicitly so the callers' code is more logical.

wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can call the popContext in the destructor, but when the object is destructed, the SplStack will be destructed just before, so no need to pop the first (so the latest) context :-). Nop?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

popContext in the descructor would not be a good design (because it might never be called, e.g. when the object is assigned e.g. to a property). Therefore I think the __construct should not have side-effects like this. The class should define in its contract that pushContext and popContext need to be called etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the first push is to set elementMap because its default value is null. So where is the problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm think you are right.. didn't read carefully enough to see that there is a "init" case for that on pushContext

@evert
Copy link
Member

evert commented Nov 18, 2014

I'd like to take a stab at this myself a bit later in the day =) there's a few other things I would like to change up a bit...

@Hywan Hywan force-pushed the stacked_contextes branch from 1dc9c05 to 3aed114 Compare November 18, 2014 16:05
@Hywan
Copy link
Member Author

Hywan commented Nov 18, 2014

@evert As you like :-). I have added inheritance and the stack includes elementMap and baseUri as requested.

@evert
Copy link
Member

evert commented Nov 18, 2014

@Hywan

I looked into SplStack. It's kind of cool, but overall I think using it is a little bit less legible. So one reason to go for it, is that it perhaps has better performance features.

So I wrote a benchmark : array vs SplStack. Doing a few pushes and pops.

End result

    Method Name   Iterations    Average Time      Ops/second
    ---  ------------  --------------    -------------
    spl: [100,000   ] [0.0000080101895] [124,840.99107]
    arr: [100,000   ] [0.0000042340684] [236,179.46311]

One thing tha'ts immediately obvious, is that the operations are so fast, that it's almost silly to optimize for. However, using a plain array is twice as fast so that means I have no argument for SplStack anymore aside from that it's kinda fun to use specialized spl objects.

I didn't test for memory, but I doubt SplStack would come out well there.

evert added a commit that referenced this pull request Nov 18, 2014
@evert evert merged commit 0132478 into sabre-io:master Nov 18, 2014
@Hywan
Copy link
Member Author

Hywan commented Nov 19, 2014

Good :-).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pushing and popping contexts

3 participants