-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Convert UniqueID::nil() to a constructor #3564
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
Conversation
pcmoritz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM conditionally on tests (esp. linting) passing.
robertnishihara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
| ClientTableDataT data; | ||
| gcs_client_2->client_table().GetClient(client_id_1, data); | ||
| RAY_LOG(INFO) << (ClientID::from_binary(data.client_id) == ClientID::nil()); | ||
| RAY_LOG(INFO) << (ClientID::from_binary(data.client_id) == ClientID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above we used .is_nil() for this, we might want to unify for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
Test FAILed. |
|
Test FAILed. |
|
Actually I prefer keeping the
But, it's good to initialize data to nil in the constructor. |
1b72cb3 to
ccf895f
Compare
|
Thanks, @raulchen. I left in |
|
Test PASSed. |
What do these changes do?
The default
UniqueID()does not initialize the ID data, which can make statements likeObjectID object_id;error-prone. This merges theUniqueID::nil()static method into the default constructor.