Refactoring table sec:accessors-command-buffer-members

[TApplencourt]

This PR refactor the Table 29. Member functions of the accessor class.

It demonstrate:

Transformation of table into section

• The name of the new section, is the name of the table (no change)
• The anchor link it's created as replacing in table id the word table. withsec: and then replacing the . with -
[[table.accessors.command.buffer.members]] -> [[sec:accessors-command-buffer-members]]

Transformation of the cell into sub section:

• The new title for each cell is {classname}::{function-name}
• The new id is named api:{section-prefix}-{function-name}. So the old [source] is replaced to [source,role=synopsis,id=api:{section-prefix}-{function-name}]
  • Particular care is taken for c++ operator, where for example operator=( is not a valid name for a anchor. For the particular =( we sanitize it to operator-asign: The new anchor is now api:accessors-command-buffer-members-operator-assign
• Merging of overload. We merge overload in the same subsection by adding a prefix (to mimic other part of the spec)
• Not showed here, but we also take care of the core indentation of the text inside the description cell.
• Each cell is separated by \n'''\n to follow current style.

Changing old reference to new one

• We take care of updating old table link to new section link

Note

• It's not the best table, as the level are so deep that we don't have a link. I can do a more "shallow" table if that help.
• Reflow mess-around a little bit the diff
• Sorry for the last "empty new-line" added. Maybe be painful to remove. It doesn't change anything in the render, but I can invest more time to try to fix it.

[CLAassistant]

All committers have signed the CLA.

[gmlueck]

Hi @TApplencourt,

Starting with accessor is quite ambitious! This is one of the most complex classes we have because of the multiple specializations, etc. This might mean that your script will need special handling that won't be necessary for the other classes. In this review, I'm concentrating mostly on the format of the output in this review. I haven't checked (yet) that the text is still correct.

I don't like the extra TOC level that the script adds now. We already have too many TOC levels in this part of the spec, so I don't want to add another. I think the TOC should look like this once this section is converted:

3.7.6.9 Buffer accessor for commands
  [Overview paragraph(s)]
  [Overview synopsis]
3.7.6.9.1 Member types
3.7.6.9.2 Constructors
3.7.6.9.3 Member functions
3.7.6.9.4 Deduction tags
3.7.6.9.5 Read only buffer command accessors and implicit conversions
3.7.6.9.6 Deprecated features of the accessor class

I also want to make sure that we convert all the sections I list above. This PR just converts "3.7.6.9.3 Member functions", and I think that makes the spec more confusing than it is now. Personally, I'd just use one PR to make all these changes. However, if you want to break it up into multiple PRs, that's OK too. However, I want to wait for all these sections to be converted before merging any of the PRs.

I think we should rename the id names (e.g. id=api:accessors-command-buffer-members-swap) to follow the same pattern we use elsewhere. That pattern is id=api:{class name}-{function name}. For example: id=api:accessor-swap. See the id names in section "SYCL runtime classes" for more examples.

I also think we should bikeshed the names of the section identifiers (e.g. [[sec:accessors-command-buffer-members]]). I'm open to other suggestions, but I think this would be more similar to what we have in "SYCL runtime classes".

[[sec:accessor]]
3.7.6.9 Buffer accessor for commands
[[sec:accessor-member-types]]
3.7.6.9.1 Member types
[[sec:accessor-ctors]]
3.7.6.9.2 Constructors
[[sec:accessor-member-funcs]]
3.7.6.9.3 Member functions
[[sec:accessor-deduction-tags]]
3.7.6.9.4 Deduction tags
[[sec:accessor-implicit-conversions]]
3.7.6.9.5 Read only buffer command accessors and implicit conversions
[[sec:accessor-deprecated]]
3.7.6.9.6 Deprecated features of the accessor class

Note that the id names and the section identifiers become part of the HTML URLs, and I'd like these to become stable across spec releases. I think it's OK to change the section identifiers as part of this reformatting, but I'd like to avoid changing them again in the future (unless, or course, the section goes away or is updated in some major way). Also, I expect people will share URL's that include these section identifiers / id names in web posts, email, etc. Therefore, I think it's better if the names are reasonably short and somewhat meaningful.

Finally, I have a nit about the callout numbers in the code synopsis. Please match the same style we user here:

https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#api:queue-single-task

• Callouts are not a comment. This saves three characters of horizontal space, which can be important for the PDF render where horizontal space is at a premium because the synopses cannot be wider than the PDF page.
• Callouts are vertically aligned within any single synopsis code block.
• Each callout is on the first line of its associated function.
• The callouts are pushed to the right such that if you drew a vertical line in the column containing the ( of the aligned callouts, that line would not intersect any characters in any of the function declarations in that synopsis code block.

[TApplencourt]

Side note on callouts (we can move into an issue as it's not related to this PR):

applenco@polaris-login-04:~/sycl_test> grep -r "// (1)" SYCL-Docs/adoc/
SYCL-Docs/adoc/chapters/programming_interface.adoc:global_ptr<value_type> get_pointer() const noexcept // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:const accessor& operator=(const value_type& other) const // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:specialization_id(const specialization_id& rhs) = delete;            // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:template <bundle_state State> // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:template <typename KernelName, bundle_state State> // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:template <bundle_state State> // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:template <typename KernelName, bundle_state State> // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:kernel_bundle<bundle_state::object> // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:bool has_kernel(const kernel_id& kernelId) const noexcept; // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:template <typename KernelName> bool has_kernel() const noexcept; // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:device_image_iterator begin() const; // (1)
SYCL-Docs/adoc/chapters/programming_interface.adoc:bool has_kernel(const kernel_id& kernelId) const noexcept; // (1)
SYCL-Docs/adoc/headers/algorithms/select.h:T select_from_group(Group g, T x, Group::id_type remote_local_id); // (1)
SYCL-Docs/adoc/headers/algorithms/permute.h:T permute_group_by_xor(Group g, T x, Group::linear_id_type mask); // (1)
SYCL-Docs/adoc/headers/algorithms/any_of.h:bool joint_any_of(Group g, Ptr first, Ptr last, Predicate pred); // (1)
SYCL-Docs/adoc/headers/algorithms/none_of.h:bool joint_none_of(Group g, Ptr first, Ptr last, Predicate pred); // (1)
SYCL-Docs/adoc/headers/algorithms/inclusive_scan.h:                            BinaryOperation binary_op); // (1)
SYCL-Docs/adoc/headers/algorithms/reduce.h:joint_reduce(Group g, Ptr first, Ptr last, BinaryOperation binary_op); // (1)
SYCL-Docs/adoc/headers/algorithms/shift.h:T shift_group_left(Group g, T x, Group::linear_id_type delta = 1); // (1)
SYCL-Docs/adoc/headers/algorithms/exclusive_scan.h:                            BinaryOperation binary_op); // (1)
SYCL-Docs/adoc/headers/algorithms/all_of.h:bool joint_all_of(Group g, Ptr first, Ptr last, Predicate pred); // (1)
SYCL-Docs/adoc/headers/hostTask/classHandler/hostTask.h:  void host_task(T&& hostTaskCallable); // (1)
SYCL-Docs/adoc/headers/hostTask/classInteropHandle/getnativeX.h:    get_native_mem(const accessor<DataT, Dims, AccMode, AccTarget, // (1)
SYCL-Docs/adoc/headers/hostTask/classInteropHandle/constructors.h:interop_handle(__unspecified__); // (1)
SYCL-Docs/adoc/headers/groups/broadcast.h:template <typename Group, typename T> T group_broadcast(Group g, T x); // (1)
SYCL-Docs/adoc/headers/groups/barrier.h:                   memory_scope scope = Group::fence_scope); // (1)

If we want to continue not-commenting and have the current indenting, we should fix the 12 who are part of adoc.

For the one part of headers (.h), it's more challenging. As for example we do

==== [code]#group_broadcast#

The [code]#group_broadcast# function communicates a value held by one work-item
to all other work-items in the group.

[source,,linenums]
----
include::{header_dir}/groups/broadcast.h[lines=4..-1]
----

  . _Constraints:_ Available only if
    [code]#sycl::is_group_v<std::decay_t<Group>># is true and [code]#T# is a
    trivially copyable type.
+
--
_Effects:_ Blocks until all work-items in group [code]#g# have reached this
synchronization point, then executes the broadcast operation.

_Synchronization:_ The call to this function in each work-item happens before
the broadcast operation begins execution.
The completion of the broadcast operation happens before any work-item blocking
on the same synchronization point is unblocked.

_Returns:_ The value of [code]#x# from the work-item with the smallest linear id
within group

But we parse the header (.h) with clang-format, so we cannot have uncommented (and keep the alignment even if commented as clang format doesn't do that). The two format are coexisting inside the current spec.

I guess we should move those away from this "include .h" + "effect addoc" pattern?

[gmlueck]

The 12 callouts you list are all in older parts of the spec, which were not written with the new format. I think we can address these as we reformat those sections.

Yes, I don't like the older style where the synopsis was in a .h file that was include-ed into the adoc file. This style makes it hard to keep the callout numbers (in the header) in sync with the specification text. This is one of the reasons the new style has the synopses directly in the adoc file rather than being include-ed.

[TApplencourt]

Before going into more details my view for those PR for this is PR is really about "automatic translation" so we can scale it to the full spec and be done with the migration from table to section.
If we start to add "manual labor", it may risk to stole those PR and not be scalable. Of course it will not be perfect, but better than the table I hope.

I also want to make sure that we convert all the sections I list above

It may required considerable amount of work (due to each table have some "non-automatic" labeling.) For example the name of the constructor in each section. But I can have a go at it.

I also think we should bikeshed the names of the section identifiers (e.g. [[sec:accessors-command-buffer-members]]). I'm open to other suggestions, but I think this would be more similar to what we have in "SYCL runtime classes".

If it's automatic then I'm ok with it. Right now, it use the name of the table to create the section name.

Note that the id names and the section identifiers become part of the HTML URLs, and I'd like these to become stable across spec releases. I think it's OK to change the section identifiers as part of this reformatting, but I'd like to avoid changing them again in the future (unless, or course, the section goes away or is updated in some major way). Also, I expect people will share URL's that include these section identifiers / id names in web posts, email, etc. Therefore, I think it's better if the names are reasonably short and somewhat meaningful.

100% agree. May conflict with the automatic approach. Maybe we can "stage" PR to a another branch (where we. do all the automatic translation, and then "manual PR fix ", review them, and then do one big merge to main.

I think we should rename the id names (e.g. id=api:accessors-command-buffer-members-swap) to follow the same pattern we use elsewhere. That pattern is id=api:{class name}-{function name}. For example: id=api:accessor-swap. See the id names in section "SYCL runtime classes" for more examples.

Good catch. Done!

Finally, I have a nit about the callout numbers in the code synopsis. Please match the same style we user here:

Done.

I don't like the extra TOC level that the script adds now.

It transformed the table into a section.
Currently 3.7.6.9.1. is Interface for buffer command accessors You propose to delete this section? I don't see how to do that automatically, so maybe out of scope of this particular PR.

[TApplencourt]

Found a potential naming in coherency, In table:

[[table.members.properties.image]]
.Member functions of the image [code]#property# classes
[width="100%",options="header",separator="@",cols="65%,35%"]
|====
@ Member function @ Description
a@
[source]
----
std::mutex* property::image::use_mutex::get_mutex_ptr() const
----
   a@ Returns the [code]#std::mutex# which was specified when
      constructing this SYCL [code]#use_mutex# property.

a@
[source]
----
context property::image::context_bound::get_context() const
----
   a@ Returns the [code]#context# which was specified when
      constructing this SYCL [code]#context_bound# property.

|====

We put the context property::image, I guess we should put. only context image::. In the queue for example we do

.[apidef]#queue::submit#
[source,role=synopsis,id=api:queue-submit]
----
template <typename T>
event submit(T cgf)
----

Not event queue::submit.

[TApplencourt]

Ok this table should be good. Minus the name of the section will do that in the next commit.