Skip to content
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

Usage of "Enumeration" in codeblock #710

Open
TApplencourt opened this issue Jan 31, 2025 · 8 comments
Open

Usage of "Enumeration" in codeblock #710

TApplencourt opened this issue Jan 31, 2025 · 8 comments
Labels
editorial Some purely editorial problem

Comments

@TApplencourt
Copy link
Contributor

Our code block, shoud be as maximun C++.

Good: https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#_special_member_functions (the (1) have been commented out)

specialization_id(const specialization_id& rhs) = delete;            // (1)
specialization_id(specialization_id&& rhs) = delete;                 // (2)
specialization_id& operator=(const specialization_id& rhs) = delete; // (3)
specialization_id& operator=(specialization_id&& rhs) = delete;      // (4)

Bad

template <int... swizzleIndexes> __writeable_swizzle__</*unspecified*/> swizzle() const  (1)

__writeable_swizzle__</*unspecified*/> XYZW_ACCESS() const                               (2)
__writeable_swizzle__</*unspecified*/> RGBA_ACCESS() const                               (3)
__writeable_swizzle__</*unspecified*/> INDEX_ACCESS() const                              (4)

This will allow us to clang-format them.

I can make a PR, if we agree that it seem to be a good idea

@TApplencourt TApplencourt added the editorial Some purely editorial problem label Jan 31, 2025
@gmlueck
Copy link
Contributor

gmlueck commented Jan 31, 2025

That adds three characters to the width of any line that has a callout. Can you instead filter these out before passing to clang-format?

@TApplencourt
Copy link
Contributor Author

TApplencourt commented Jan 31, 2025

Good point for the size. clang-format will split the line if it is too long:

applenco@aurora-uan-0011:~/SYCL-Docs> cat test.cpp
specialization_id(const specialization_id& rhs) aaaaaaaaaaaaaaaaaaaaaaaaaa   = delete;                 // (1)
specialization_id(specialization_id&& rhs) = delete; // (2)
specialization_id& operator=(const specialization_id& rhs) = delete; // (3)
specialization_id& operator=(specialization_id&& rhs) = delete;      // (4)
applenco@aurora-uan-0011:~/SYCL-Docs> clang-format test.cpp
specialization_id(const specialization_id& rhs)
    aaaaaaaaaaaaaaaaaaaaaaaaaa = delete;                              // (1)
specialization_id(specialization_id&& rhs) = delete;                  // (2)
specialization_id& operator=(const specialization_id& rhs) = delete;  // (3)
specialization_id& operator=(specialization_id&& rhs) = delete;       // (4)

Yes, it's not a technical problem ( I already have a regex that checks for that), but the fact that some code blocks will not be formatted adds more potential "formatting bugs."

@Pennycook
Copy link
Contributor

We could also consider making these into table environments, to separate the code and non-code, something like:

[cols=".^1,.^1",frame=all,grid=none,separator="@"]
|====
a@
[source]
----
template <int... swizzleIndexes> __writeable_swizzle__</*unspecified*/> swizzle() const
----
a@
(1)

a@
[source]
----
__writeable_swizzle__</*unspecified*/> XYZW_ACCESS() const
----
a@
(2)

a@
[source]
----
__writeable_swizzle__</*unspecified*/> RGBA_ACCESS() const
----
a@
(3)

a@
[source]
----
__writeable_swizzle__</*unspecified*/> INDEX_ACCESS() const
----
a@
(4)
|====

This currently produces the below, but I suspect that messing around with column padding and other formatting stuff could reduce the whitespace and hide the fact it's a table:

Image

One thing I like about this is that all the numbers are naturally aligned, so we don't have to keep realigning them when we make a change to the function names.

@gmlueck
Copy link
Contributor

gmlueck commented Feb 3, 2025

I thought you hated Asciidoc tables, @Pennycook ...

@Pennycook
Copy link
Contributor

Pennycook commented Feb 3, 2025

I thought you hated Asciidoc tables, @Pennycook ...

Oh, I do 😆 -- I think the syntax is awful. But this seemed like it could be a good way to separate the thing that we want to format (in the [source] block) from descriptive information. That seems like a good idea, for several reasons.

There's probably an even smarter way to do it that wouldn't require tables, but I'm not very good at AsciiDoc. I know you've created custom environments in the past... Maybe there's a way to automatically generate these numbers and keep them out of the code?

@gmlueck
Copy link
Contributor

gmlueck commented Feb 3, 2025

To be honest, it seems like low ROI to me. I think our current formatting with the "new format" synopses looks good. If we move to tables (or something else), we'd just be investing time to try to get the formatting to look as good as it does already.

I think the motivation here is to extract the callouts, so @TApplencourt can run clang-format over the remaining source code. It seems like it would be relatively easy to extract the callouts with a regexp. In addition, I suspect clang-format will work better if it sees all the code in the synopsis as a single code block, rather than running each function declaration through clang-format separately (which is what would happen if each function declaration was a separate row in the table).

@TApplencourt
Copy link
Contributor Author

One thing I like about this is that all the numbers are naturally aligned, so we don't have to keep realigning them when we make a change to the function names.

So @Pennycook clang-format magically aligns the (1), (2). (I don't know why and how, but they do. See my previous example, where after running clang-format everything is nicely aligned). So it's not "naturally" aligned, but at least not "manually" aligned :)

In addition, I suspect clang-format will work better if it sees all the code in the synopsis as a single code block, rather than running each function declaration through clang-format separately (which is what would happen if each function declaration was a separate row in the table).

If we don't put ; at the end of the enumeration, it's not valid C++, so clang-format does weird stuff. (even more in enumeraton, where he think everything is just one big line).

My only goal is to try to use "valid" C++ as much as possible. It just improves the overall tooling experience (for example, we can imagine, running all the code examples, in a CI one day. And the code block to the front end, so see if we made a typo or something. Just some idea)

@gmlueck
Copy link
Contributor

gmlueck commented Feb 3, 2025

If we don't put ; at the end of the enumeration, it's not valid C++, so clang-format does weird stuff. (even more in enumeraton, where he think everything is just one big line).

That makes sense. We can add trailing semicolons to the function definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Some purely editorial problem
Projects
None yet
Development

No branches or pull requests

3 participants