-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[core] Implement dup2 wrapper #50439
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: dentiny <dentinyhao@gmail.com>
2fa5410
to
75db43a
Compare
Signed-off-by: hjiang <hjiang@anyscale.com>
src/ray/util/BUILD
Outdated
"@platforms//os:windows": ["dup2_wrapper_windows.cc"], | ||
"//conditions:default": ["dup2_wrapper_posix.cc"], |
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.
What's the difference between this approach and the other approach the current codebase is using which is ifdef
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.
No difference,
select
clause works at file level,ifdef
works at source code level- I use
select
here because implementation for two platforms are completely different and almost nothing to share
Signed-off-by: dentiny <dentinyhao@gmail.com>
@@ -313,6 +313,8 @@ class RAY_EXPORT Status { | |||
// Returns the string "OK" for success. | |||
std::string ToString() const; | |||
|
|||
std::string StatusString() const { return ToString(); } |
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.
What's the difference between StatusString and ToString
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.
Functionality-wise no difference, the reason to have this function is for pretty print status for GTEST macros.
In nuro, we have another way to handle:
template <typename Status>
struct ConsumeStatus {...};
template <>
struct ConsumeStatus<Status> {...};
template <>
struct ConsumeStatus<StatusOr> {...};
// other status classes to handle
Having a StatusString
is easier to implement.
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.
Could you elaborate, I don't think I get it. Why cannot the caller call ToString()
?
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.
We use it in the macro
CHECK_OK(s) << s.StatusString();
it's a macro thus duck typing, so we have to have the same function name for both Status
and StatusOr
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.
Calling ToString
is OK for Status
, but improper for StatusOr
, because we haven't implement the stringify function for value-type
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
…o hjiang/dup2-with-restore
Signed-off-by: dentiny <dentinyhao@gmail.com>
@jjyao The failed CI test has nothing to do with my change, which hasn't been integrated with python yet. |
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
@jjyao This PR should be good to go, the only change related to windows platform is C++ side unit tests, which already passed. I don't know why my CI gets stuck for a day. |
Hi @jjyao I think this PR should be good to go. |
It's used in logging redirection to achieve
dup2