Repository navigation
[SYCL][SYCL2020] Add missing noexcept to ctors of public SYCL RT classes - #23184
Conversation
…asses Note: this patch must be merged AFTER this one: - KhronosGroup/SYCL-Docs#1044 This patch add missing `noexcept` to some of the classes in accordance with SYCL 2020 spec update above.
There was a problem hiding this comment.
🟡 Changes recommended
The test cannot detect a missing move constructor when a non-throwing copy constructor accepts rvalues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Implements a pending SYCL 2020 update requiring non-throwing move operations for public runtime classes.
Changes:
- Marks
deviceandbuffermove constructorsnoexcept. - Adds explicit move operations to
h_item. - Adds compile-time checks for common-semantics classes.
File summaries
| File | Description |
|---|---|
sycl/include/sycl/buffer.hpp |
Makes buffer moves non-throwing. |
sycl/include/sycl/device.hpp |
Updates the public device declaration. |
sycl/source/device.cpp |
Updates the device implementation. |
sycl/include/sycl/h_item.hpp |
Adds move constructor and assignment. |
sycl/test/basic_tests/common_semantics_noexcept.cpp |
Tests move-operation requirements. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| static_assert(std::is_nothrow_move_constructible_v<__VA_ARGS__>, \ | ||
| #__VA_ARGS__ " must be nothrow move constructible"); \ |
Robertkq
left a comment
Robertkq
left a comment
There was a problem hiding this comment.
In general, LGTM
I did not check if other classes not modified by this PR need noexcept, although testing side of this PR seems to test everything.
My understanding is that all classes should define (even if = default), move constructor & move assignment operator with explicit noexcept, even if most compilers would implicitly add noexcept.
Also a little bit related, I've tried to extend the explicit noexcept to the destructor, but the decision was to leave out the explicit noexcept in the case of the destructor, perhaps this PR (or some other, no issue) could add the default destructor for classes that do not explicitly define one? (reiterating, no explicit noexcept needed for =default destructor)
There was a problem hiding this comment.
| h_item(h_item &&hi) noexcept = default; |
There was a problem hiding this comment.
| h_item &operator=(h_item &&hi) noexcept = default; |
|
Merging as is to fix the nightly failure. @Robertkq would you be interested to create a follow-up patch to address your concerns? |
Hi Nikita, thanks for pinging me on this, forgot about it. I will try and open an issue on it later today for which I will analyze other classes that might not adhere to the added specifications :) I'll probably end up implementing it as well if there is no issue around that! |
Would be great, thanks! |
This patch adds missing
noexceptto some of the classes in accordance with SYCL 2020 spec update above.Note: this patch must be merged only AFTER this one: