Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize with defer logic #21

Merged
merged 10 commits into from
Jul 18, 2022
Merged

optimize with defer logic #21

merged 10 commits into from
Jul 18, 2022

Conversation

djunny
Copy link
Contributor

@djunny djunny commented Apr 11, 2022

No description provided.

djunny added 2 commits April 12, 2022 00:56
1. optimize destruct method
2. return SplStack Instance
add examples for php 7.4
@djunny djunny changed the title optimize defer logic optimize with defer logic Apr 11, 2022
src/functions.inc.php Outdated Show resolved Hide resolved
@bkrukowski
Copy link
Member

What's the goal of this pull request?

@djunny
Copy link
Contributor Author

djunny commented Jul 18, 2022

less memory useage & higher performance & chain call & better implementation

@bkrukowski
Copy link
Member

@djunny

Chain calls

Chain calls look a bit weird to me. I cannot see a big benefit of using

defer($_, $callbackA)->push($callbackB);

instead of

defer($_, $callbackA);
defer($_, $callbackB);

Furthermore, why do we have to register 2 callbacks in consecutive lines? Would not be easier to have just:

defer($_, function() {
    $callbackB();
    $callbackA();
});

?

In my opinion, the above code is more explicit and easier to read. I appreciate your effort, but I have to reject this part.

Design

I really like your way of making the order of callbacks explicit.

            while ($this->count() > 0) {
                \call_user_func($this->pop());
            }

If you are ok with reverting "chain calls" from your pull request, I would like to merge it.

@djunny
Copy link
Contributor Author

djunny commented Jul 18, 2022

chain calls:

I just return the SplStack object on the basis of the original,

but i dont think it is recommended to use it like that.

so i removed in README

@@ -9,23 +9,17 @@
* file that was distributed with this source code.
*/

function defer(?SplStack &$context, callable $callback): void
function defer(?SplStack &$context, callable $callback): SplStack
Copy link
Member

Choose a reason for hiding this comment

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

I cannot approve changing the header of this function, I explained it in the previous comment:

Suggested change
function defer(?SplStack &$context, callable $callback): SplStack
function defer(?SplStack &$context, callable $callback): void

README.md Outdated
Comment on lines 30 to 43

// in 7.4+
// without use statments
defer($_, fn() => print_r([1]));

// 7.3 -
defer($_, function () {
echo "goodbye\n";
});

defer($_, function () {
echo "...\n";
});

Copy link
Member

Choose a reason for hiding this comment

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

Please revert those changes

@bkrukowski bkrukowski merged commit 1aadc6a into php-defer:5.0 Jul 18, 2022
bkrukowski added a commit that referenced this pull request Jul 25, 2022
bkrukowski added a commit that referenced this pull request Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants