[core] Implement dup2 wrapper #50439
Conversation
Signed-off-by: dentiny <dentinyhao@gmail.com>
dentiny
force-pushed
the
hjiang/dup2-with-restore
branch
from
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.
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.
No difference,
selectclause works at file level,ifdefworks at source code level- I use
selecthere 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.
What's the difference between StatusString and ToString
There was a problem hiding this comment.
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.
Could you elaborate, I don't think I get it. Why cannot the caller call ToString()?
There was a problem hiding this comment.
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.
Calling ToString is OK for Status, but improper for StatusOr, because we haven't implement the stringify function for value-type
There was a problem hiding this comment.
Got it. Please turn the above explanation to a comment so people know why you need two.
There was a problem hiding this comment.
Why need a comment here? Exposing status stringify seems natural to me.
There was a problem hiding this comment.
Anyway I added a comment for necessity from duck-type template/macro.
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. |
| @@ -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.
Got it. Please turn the above explanation to a comment so people know why you need two.
Signed-off-by: dentiny <dentinyhao@gmail.com>
|
The failed |
It's used in logging redirection to achieve
dup2