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

Explicitly write down the rule of architecture extension test macros #28

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kito-cheng
Copy link
Collaborator

We didn't write the rule explicitly, so people might not expect those marco
to be available if it's not appear on the table.

So let document that, and add more note to say people can expect that
even not list on the table.

We didn't write the rule explicitly, so people might not expect those marco
to be available if it's not appear on the table.

So let document that, and add more note to say people can expect that
even not list on the table.
@asb
Copy link
Contributor

asb commented Feb 15, 2022

I've left a couple of minor phrasing suggestions.

One alternate approach would be to cut down the table and just list a few examples, on the basis that keeping it up to date is extra work that might not have much value for end users.

@kito-cheng
Copy link
Collaborator Author

kito-cheng commented Feb 16, 2022

@asb Seems like those suggestion are missing by some accident? Did you mind reply those suggestion again?

@@ -102,6 +102,14 @@ availability and version for certain extension, however not all compilers are
supported, so you should check `__riscv_arch_test` to make sure this compiler
is supporting those preprocessor definitions.

The naming rule of architecture extension test macros is `__riscv_<ext_name>`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"naming rule of" => "naming rule for"

@@ -130,6 +138,11 @@ For example:
| __riscv_zbs | Arch Version | `Zbs` extension is available. |
| __riscv_zfh | Arch Version | `Zfh` extension is available. |

NOTE: The table might not list all architecture extension test macros for
ratified extensions due to not up-to-date, and might not list unratified
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps just simplify to: "NOTE: The table may not be exhaustive, as it might not be fully up-to-date with all ratified architecture extensions and may not list unratified extensions."

@asb
Copy link
Contributor

asb commented Feb 16, 2022

@asb Seems like those suggestion are missing by some accident? Did you mind reply those suggestion again?

Ah, they were still pending and unsubmitted. Fixed now.

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

Successfully merging this pull request may close these issues.

2 participants