-
Notifications
You must be signed in to change notification settings - Fork 68
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
Update queue description to new format #606
Conversation
Update the section describing the `queue` class to the new format.
Thanks will review this week! |
@@ -53,22 +53,25 @@ class queue { | |||
|
|||
bool is_in_order() const; | |||
|
|||
template <typename Param> typename Param::return_type get_info() const; | |||
template <typename Param> |
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.
It's not important, but AFAIK those files were formatted via clang format.
In the future, we should commit a clang-format-config on we a style we like and stick with it.
a@ | ||
[source] | ||
_Effects:_ Submit a <<command-group-function-object>> to the queue, in order to | ||
be scheduled for execution on the device. |
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.
Missing the Synchronous errors are reported through SYCL exceptions.
?
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.
The text I have here describing the submit
function is exactly the same as the text prior to this PR. The only thing I added was the word "Effects".
I do agree that the submit
function could throw a synchronous exception if there is an error condition, but the sentence you propose was not there before. Are you suggesting that we should add it as part of this PR?
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 for the clarification.
We can do it in a follow-up PR. Cleaner indeed
Improve the wording of the event that is returned by `queue::submit` and friends. Since the new wording says that the event represents the *command* that is submitted to the queue, it seemed awkward for the _Effects_ paragraph to say that a *command group function* is submitted to the queue. I replaced this with wording saying that the command group function is immediately called, which is consistent with what we say in the introductory paragraphs of section 4.6.5 "Queue class". The new wording also notes that the command group function may submit no more than one command to the queue, which is consistent with the existing wording in section 4.9.3 "Command group scope".
These sentences had two clauses, but one was passive voice while the other was active voice. Change them so both clauses are passive voice.
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!
WG approved to merge. |
Cherry pick KhronosGroup#606 from main (cherry picked from commit 23f8a15)
Update queue description to new format (cherry picked from commit 23f8a15)
Update the section describing the
queue
class to the new format.