PeclEvLoop: Add new PeclEvLoop (PECL ext-ev) by ivkalita · Pull Request #97 · reactphp/event-loop

ivkalita · 2017-04-08T15:43:37Z

Improved and (just a little bit) refactored #25.

The existing libev driver seems to function only with a non-official, out of date libev extension for PHP. This PeclEvLoop class is a very similar clone of the LibEvLoop library but for the officially documented libev extension. Changes are mostly to constants and function parameter order.

ivkalita · 2017-04-08T17:04:24Z

Sorry, I forgot to add travis configuration for PECL ev extension (which is necessary for PeclEvLoop). Will update it soon.

WyriHaximus · 2017-04-08T18:21:37Z

@kaduev13 tbh no need to close your PR for that, just add another commit fixing that. Or just squash it in your last commit. (@clue is our resident Git guru)

ivkalita · 2017-04-08T19:05:32Z

@WyriHaximus ok, thank you. I'm very new to open-source, but I will learn fast ;) Btw, it's fixed now.

jsor · 2017-04-16T21:07:36Z

+ /**
+ * {@inheritdoc}
+ */
+ public function tick()


Note, that tick() has been removed from the interface (see #72).

@jsor, oh, thank you! Fixed in c4dbd97.

jsor

Minor nitpicking, otherwise LGTM.

jsor · 2017-04-20T17:50:34Z

+ /**
+ * {@inheritdoc}
+ */
+ public function addReadStream($stream, callable $listener)


The docblocks containing just {@inheritdoc} can be removed.

@jsor, no problem, I can remove it, but {@inheritdoc} makes inheritance explicit (link to PSR-5 inheritance section). Is it a reactphp project convention?

Since this method implements the interface, it's explicit that the docblock is inherited. We tend to avoid docblocks since they are often redundant and become easily out of date. With type hints and return types in PHP 7, they will become even more obsolete (at least 99.9% of the time) :)

@jsor, ok, thank you for the explanation 😃 , I agree with your arguments (but I just looked through LibEvLoop, StreamSelectLoop and other loops sources and there are a lot of "only @inheritdoc" PHPDocs).
I removed unnecessary @inheritdoc blocks from PeclEvLoop in 7128aaf.
Btw, I think that it'll be better to cleanup the history of this PR before merging it into master (cause of a lot of little fix commits). I will do it tomorrow morning (UTC).

jsor · 2017-04-20T17:51:29Z

+/**
+ * @author Ivan Kalita kaduev13@gmail.com
+ */
+


We usually don't add @author doc blocks.

@jsor, thanks, I'll keep that in mind next time 😄. I removed them in 54de798.

…nd cancelTimer.

jsor · 2017-04-25T18:04:44Z

ping @WyriHaximus and @clue

WyriHaximus · 2017-05-24T16:58:49Z

ping @clue

WyriHaximus · 2017-07-26T10:59:50Z

Been running this against the memory benchmark from #59 the past few hours and haven't ran into any leaks yet. /cc ping @clue

clue

Thanks @kaduev13! 👍

Functionally this looks good to me, but I'd like to address a few minor points before getting this:

Name prefix Pecl/Ext/Lib is inconsistent
17 commits should probably be squashed to a reasonable number.

clue · 2017-07-26T11:50:06Z

+use SplObjectStorage;
+
+/**
+ * @see https://bitbucket.org/osmanov/pecl-ev/overview


A short description would help?

clue · 2017-07-26T11:51:03Z

+ *
+ * @return \Closure
+ */
+ private function getStreamListenerClosure($stream, callable $listener) {


Minor nitpick: Line break before { (PSR-2)

ivkalita · 2017-07-26T11:57:09Z

@clue Thank you for the corrections, I'll fix these issues on this weekend 😃

WyriHaximus · 2017-08-23T19:38:49Z

@kaduev13 any luck with those corrections?

clue · 2018-02-07T08:20:16Z

Superseded by #148 👍

markkimsaland others added 8 commits April 8, 2017 17:45

Document new driver.
051a4a3

Replace "EvLoop::defaultLoop()" with "new EvLoop()".
0b375f7

Fix PeclEvLoop SplObjectStorage item retrieving method.
3391da5

Remove unnecessary "use" statements from PeclEvLoop.
0e56b44

Remove nextTickQueue from PeclEvLoop.
316e6a9

Rename PeclEvLoop private members in StreamSelectLoop way.
4c70da0

Prevent PeclEvLoop from updating existing streams callbacks.
3ae1829

ivkalita closed this Apr 8, 2017

ivkalita reopened this Apr 8, 2017

jsor reviewed Apr 16, 2017
View reviewed changes

jsor requested review from WyriHaximus and clue April 20, 2017 17:43

jsor approved these changes Apr 20, 2017
View reviewed changes

ivkalita added 9 commits April 21, 2017 12:09

Add early returns in PeclEvLoop removeReadStream, removeWriteStream a…
52e5e2e
…nd cancelTimer.

Make PeclEvLoop::run more verbose.
5392032

Add PeclEvLoop::__destruct() to prevent SEGFAULT.
45b7ab0

Remove unnecessary PeclEvLoop::tick().
42bc95d

Remove unnecessary "@inheritdoc" from PeclEvLoop.
dac7c41

Add base test for canceling non-existent timer.
41d9d63

Add PeclEvLoop test.
2ef8ff0

Add PeclEvLoopTimerTest.
54710a3

Add PECL ev extension to travis-init.sh.
79dd25c

ivkalita force-pushed the pecl-ev-loop branch from 7128aaf to 79dd25cCompare April 21, 2017 09:10

WyriHaximus approved these changes Apr 25, 2017
View reviewed changes

clue added the new feature label Apr 25, 2017

WyriHaximus added this to the v0.5.0 milestone Jul 26, 2017

clue reviewed Jul 26, 2017
View reviewed changes

WyriHaximus modified the milestones: v0.5.1, v0.5.0 Sep 5, 2017

clue changed the title ~~PeclEvLoop: Add new PeclEvLoop (pecl ev) – improved #25~~PeclEvLoop: Add new PeclEvLoop (PECL ext-ev)Dec 4, 2017

This was referenced Dec 4, 2017
ExtEvLoop: Add new ExtEvLoop (PECL source ext-ev) #12
Closed
PeclEvLoop: Add new PeclEvLoop (PECL ext-ev) #25
Closed

ivkalita mentioned this pull request Jan 15, 2018
Implement ExtEvLoop (PECL ext-ev) #148
Merged

clue removed this from the v0.5.1 milestone Feb 7, 2018

clue closed this Feb 7, 2018

Uh oh!

Uh oh!

Conversation

ivkalita commented Apr 8, 2017

Uh oh!

ivkalita commented Apr 8, 2017

Uh oh!

WyriHaximus commented Apr 8, 2017

Uh oh!

ivkalita commented Apr 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsor left a comment

Choose a reason for hiding this comment

Uh oh!

jsorApr 20, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsorApr 20, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsor commented Apr 25, 2017

Uh oh!

WyriHaximus commented May 24, 2017

Uh oh!

WyriHaximus commented Jul 26, 2017

Uh oh!

clue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivkalita commented Jul 26, 2017

Uh oh!

WyriHaximus commented Aug 23, 2017

Uh oh!

clue commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ivkalita commented Apr 8, 2017•
edited
Loading

jsorApr 20, 2017•
edited
Loading

jsorApr 20, 2017•
edited
Loading

clue left a comment •
edited
Loading