Separate more interface and definition. Add comments on std::future. Mark noexcept to compat mode-related functions#588
Conversation
kingcrimsontianyu
added
improvement
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
|
/ok to test |
madsbk
left a comment
madsbk
left a comment
There was a problem hiding this comment.
Looks good, I only have a minor suggestion
| { | ||
| if (closed()) { return; } | ||
|
|
||
| if (!is_compat_mode_preferred()) { cuFileAPI::instance().HandleDeregister(_handle); } |
There was a problem hiding this comment.
Unrelated to this refactoring, but might make sense to fix in this PR:
We need an exception catch or make is_compat_mode_preferred() and compat_mode() noexcept as well.
There was a problem hiding this comment.
This is interesting: defaults::compat_mode() can't be noexcept because it calls defaults::instance() and defaults's constructor is potentially throwing. One workaround is:
CompatMode defaults::compat_mode() noexcept {
try {
return instance()->_compat_mode;
} catch(...) {}
return CompatMode::ON; // Or we may add an CompatMode::Invalid?
}But is it worth the hassle to make it noexcept at all?
@wence- @madsbk
There was a problem hiding this comment.
But is it worth the hassle to make it noexcept at all?
Heh, maybe not :)
There was a problem hiding this comment.
Side note, we can use noexcept on functions that might throw exceptions, it just makes them fatal.
There was a problem hiding this comment.
It's UB, rather than just making things fatal
There was a problem hiding this comment.
It's UB, rather than just making things fatal
It is UB in the sense that calling std::terminate() is UB (it is implementation-defined whether any stack unwinding).
But std::terminate() is guaranteed to be called when violating noexcept: https://en.cppreference.com/w/cpp/error/terminate
There was a problem hiding this comment.
Ah sorry, I was wrong, it is not UB, so all good.
kingcrimsontianyu
changed the title
kingcrimsontianyu
changed the title
| try { | ||
| cuFileAPI::instance(); | ||
| } catch (const std::runtime_error&) { | ||
| } catch (...) { |
There was a problem hiding this comment.
Imho ... is better since it simplifies the effort to cover all possible exception types that the try block may generate (the exact types can be documented in cuFileAPI::instance()'s @throw section) while satisfying the noexcept promise. But I'm ready to revert if there's any other consideration I'm not aware of. @madsbk @vuule @wence-
| _fd_direct_on = -1; | ||
| _fd_direct_off = -1; | ||
| _initialized = false; | ||
| } catch (...) { |
There was a problem hiding this comment.
cuFileAPI constructor (hence cuFileAPI::instance()) is potentially throwing. So to satisfy the noexcept promise, the try-catch block has to be here.
| bool is_running_in_wsl() noexcept | ||
| { | ||
| struct utsname buf {}; | ||
| int err = ::uname(&buf); | ||
| if (err == 0) { | ||
| const std::string name(static_cast<char*>(buf.release)); | ||
| // 'Microsoft' for WSL1 and 'microsoft' for WSL2 | ||
| return name.find("icrosoft") != std::string::npos; | ||
| try { | ||
| struct utsname buf {}; | ||
| int err = ::uname(&buf); | ||
| if (err == 0) { | ||
| const std::string name(static_cast<char*>(buf.release)); | ||
| // 'Microsoft' for WSL1 and 'microsoft' for WSL2 | ||
| return name.find("icrosoft") != std::string::npos; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
question: what might throw here?
There was a problem hiding this comment.
I was a bit paranoid about STL function calls. Here the string's constructor basic_string(const charT* s, const Allocator& a = Allocator()); should be potentially throwing, because it does not have a "Throws" section in its specs. According to the specs:
Functions defined in the C++ standard library that do not have a Throws: paragraph but do have a potentially-throwing exception specification may throw implementation-defined exceptions.
In particular, they can report a failure to allocate storage by throwing an exception of type bad_alloc, or a class derived from bad_alloc
I would imagine the rare but not impossible case in practice, where the stack may overflow (when short string optimization is in effect), or the heap may run out of memory.
The string find overload size_type F(const charT* s, size_type pos) const; is on the same boat.
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
One query about the catch in is_running_in_wsl, otherwise looks good!
|
/merge |
3 participants
This PR performs makes the following three improvements:
file_handle.hppthat was missed in the previous PR Continue to make KvikIO a shared library by moving code from hpp to cpp #581.pread/pwrite:noexceptspecifier to compatibility mode-related functions.