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

Receive from closed channel #8175

Merged
merged 10 commits into from
Feb 6, 2018

Conversation

wangkuiyi
Copy link
Collaborator

No description provided.

kavyasrinet
kavyasrinet previously approved these changes Feb 6, 2018
Copy link

@kavyasrinet kavyasrinet left a comment

Choose a reason for hiding this comment

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

LGTM. I just pushed something similar in my latest commit after your comment. Let's merge this.

abhinavarora
abhinavarora previously approved these changes Feb 6, 2018
Copy link
Contributor

@abhinavarora abhinavarora left a comment

Choose a reason for hiding this comment

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

LGTM!

@wangkuiyi
Copy link
Collaborator Author

@kavyasrinet I am afraid that I will be at the meeting for the whole night, so might not have time to polish this PR, which was originally created only for your reference. Please feel free to commit to my branch of this PR to fix the CI problems. Thanks!

@kavyasrinet
Copy link

Sure, will do.

@kavyasrinet kavyasrinet dismissed stale reviews from abhinavarora and themself via c67c424 February 6, 2018 03:02
EXPECT_EQ(ch->Send(&i), true); // sending should not block
}

int out;
Copy link
Contributor

Choose a reason for hiding this comment

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

int out ==> size_t out

@@ -119,6 +150,7 @@ TEST(Channel, BufferedChannelCloseUnblocksReceiversTest) {
int data;
// All reads should return false
EXPECT_EQ(ch->Receive(&data), false);
EXPECT_EQ(data, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think the data should be zero when the channel was closed. If the element of channel is Tensor* and the channel has been closed, what will the data be after ch->Receive(&data)

Copy link

@kavyasrinet kavyasrinet Feb 6, 2018

Choose a reason for hiding this comment

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

it should be the zero value of that return type, as mentioned here: https://dave.cheney.net/2014/03/19/channel-axioms
Here are the zero values: https://golang.org/ref/spec#The_zero_value

For now I have removed this in my current commit, but once we decide we can introduce that behavior again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem here is that C++ syntax doesn't define zero value of all types like Go does. So, basically, we have no idea how to write a C++ code snippet to clear the value of any data type to zero.


for (size_t i = 0; i < buffer_size; ++i) {
EXPECT_EQ(ch->Receive(&out),
false); // after receiving residual values, return zeros.
Copy link
Contributor

Choose a reason for hiding this comment

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

This annotation should be removed or changed.
// after receiving residual values, return zeros.

Copy link
Contributor

@chengduoZH chengduoZH left a comment

Choose a reason for hiding this comment

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

LGTM

@wangkuiyi wangkuiyi merged commit 6024a17 into PaddlePaddle:develop Feb 6, 2018
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.

4 participants