Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move non-normative parts of "A" extensions (atomics) into NOTE block #1822

Merged
merged 2 commits into from
Jan 25, 2025

Conversation

radimkrcmar
Copy link
Contributor

Most of the series moves non-normative text into notes, but the last patch also removes page breaks before examples as I found it hard to read otherwise.

It doesn't bring the ISA manual any closer to being ISO worthy, but I hope it'll at least be easier to understand until then.
I stumbled upon this issue thanks to #1814 and tried hard to avoid any controversial changes.

@radimkrcmar
Copy link
Contributor Author

Looking at this again, patches for examples can be squashed. I'll do that for the next version if there are no objections.

@wmat
Copy link
Collaborator

wmat commented Jan 23, 2025

What is the reasoning for moving the blocks of text to be non-normative? In the original spec this text was normative.

@gfavor
Copy link
Collaborator

gfavor commented Jan 24, 2025

I would agree with @wmat. Changing normative text in a ratified spec to non-normative text by default is not allowed since that represents a semantic arch spec change.

Now I recognize that some of these cases fall in the grey area of, for example, making suggestions and usage observations. But even if such changes to non-normative text are ultimately deemed acceptable, this will require review by others and probably official review by the Arch Review Committee (as would be true with changes to other ratified specs that aren't simply typo fixes and clarifications).

So, as a first level of triage (to determine whether there's any reasonable chance of approval by the ARC so as to warrant the effort of requesting and conducting an official review), @aswaterman should comment on these changes before we go any further.

src/a-st-ext.adoc Outdated Show resolved Hide resolved
src/a-st-ext.adoc Outdated Show resolved Hide resolved
@aswaterman
Copy link
Member

@gfavor Indeed, a subset of this PR is the right thing to do, but not the entirety. Comments above.

@gfavor
Copy link
Collaborator

gfavor commented Jan 24, 2025

@aswaterman Thanks. Just as I expected.

@radimkrcmar
Copy link
Contributor Author

For the general motivation. I heard that the goal is ISO certification, which implies a lot of rewriting, so I find it wise to first properly mark the parts that don't need as much attention.

In this series I only aim to do changes that do not change any behavior.

Examples are usually in note blocks already.  Make these consistent.

The text about compare-and-swap example should probably be improved and
moved above the example.

Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
We should improve the tooling instead of adding manual page breaks.

Also replace "above" with a reference, because "above" is not correct
anymore and most importantly, it's not acceptable for a spec.

This does not imply any acceptability of the the rest of the sentence,
I just didn't want to leave the spec in a worse shape.

Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
@radimkrcmar
Copy link
Contributor Author

I have updated the pull request.

v1->v2:

  • Remove [1/5] and [4/5] (aswaterman)
  • Squash [2/5] and [3/5]

This removes the controversial changes and the current result can be seen on screenshots that were provided during v1 review.

Copy link
Member

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

This version LGTM; thanks, @radimkrcmar.

@aswaterman aswaterman merged commit 91b80d3 into riscv:main Jan 25, 2025
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants