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

Improve the instruction documentation #1561

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Nov 25, 2024

Fixes #1560:

  • Clarifies the fragile behavior of STOP's second byte.
  • Describes u3 as a bit index.
  • Mentions "bitwise NOT" as a synonym for "complement".
  • Describes the act of loading a register to itself, including emulator-specific functionality.

And does more cleanup:

  • Specifies explicit ranges for all numeric constant ranges.
  • Consistently puts a period at the end of abbreviation descriptions.
  • Separates instruction details/clarifications from their basic functionality with newlines.
  • Consistently uses "the".
  • Consistently says "Store assignee into destination" (which a few descriptions already said), not "Load assignee into destination", "Load destination with assignee", "Load value into destination from assignee", etc.
  • Consistently uses uppercase instructions.
  • Consistently refer to "double speed mode", not "dual speed mode".

@Rangi42 Rangi42 added docs This affects the documentation (web-specific issues go to rgbds-www) rgbasm This affects RGBASM labels Nov 25, 2024
@Rangi42 Rangi42 added this to the v0.9.0 milestone Nov 25, 2024
@Rangi42 Rangi42 requested a review from ISSOtm November 25, 2024 02:48
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Thank you for this thorough improvement! I have comparatively few nitpicks.

I wonder what @nummacway thinks?

man/gbz80.7 Outdated Show resolved Hide resolved
@@ -782,9 +814,9 @@ and
bytes away.
For example:
.Bd -literal -offset indent
JR Label ; no-op; encoded offset of 0
JR Label ; no-op; encoded offset of 0
Copy link
Member

Choose a reason for hiding this comment

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

Oh, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other code samples weren't indented, and with just three lines, "indented unindentedLabel indented" looked odd to me.

Copy link
Member

Choose a reason for hiding this comment

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

I think it helps making the label stand out from the instructions, especially in the absence of syntax highlighting.

If you think the other samples should be indented, then I'll agree.

.Sy NOP
byte as the second
.Sy STOP
byte by default if one is not specified.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps an example should be given of how to do so? Otherwise, it's not clear that the syntax is stop $AF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The opcode table documents it as "STOP n8"; should we do the same here to make that clear?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the canonical definition as just STOP, and mention the optional STOP n8 syntax in the description, since it's really uncommon.

Copy link
Member

Choose a reason for hiding this comment

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

which is why
.Xr rgbasm 1
allows explicitly specifying the second byte
.Pq Sy STOP Ar n8
to override the default of $00.

Choose a reason for hiding this comment

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

Please turn to override the default of $00. into to override the default of $00 (NOP). and I'm fine with the latest suggestions.

@nummacway
Copy link

Damn the diff is impossible to read with just one word per line. How do manpage maintainers do this?

Two comments of the things I can decipher:

  • Please don't call the bit index bidx, that sounds odd. I think this was a suggestion that did not make it to the PR. It was in an email from Github but I don't know why. I'm okay with all other options, including keeping it at u3.
  • The last line, "byte by default if one is not specified", sounds really odd, but I'm yet to come up with a good alternative.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Nov 25, 2024

Thanks for your feedback!

  • My own preference is to keep u3 since it's an accurate spec of what a bit index is, and its description of "3-bit unsigned bit index" clarifies its meaning.
  • Yeah, I was having a hard time phrasing that line well. Happy to change it, particularly if we document STOP as STOP n8. (Or STOP [n8] since the parameter is optional, but then it looks like dereferencing a memory location...)

@nummacway
Copy link

nummacway commented Nov 25, 2024

One important thing: The "bitwise NOT" was not meant for CCF but CPL. Because CCF affects a flag which is basically a boolean, CCF would be called the "logical NOT". I personally wouldn't even use a term here at all, but say that this "inverts" or "flips the carry flag", though I'm not sure if this wording makes sense to other people and where to put this.

@nummacway
Copy link

You could also call CPL the "subtraction from 255" (A = 255-A).

@Rangi42
Copy link
Contributor Author

Rangi42 commented Nov 25, 2024

CPL is the ComPLement (aka bitwise NOT) of register A. CCF is the Complement of the Carry Flag (again aka bitwise NOT, or logical negation in this case since it's one bit).

@ISSOtm
Copy link
Member

ISSOtm commented Nov 26, 2024

Damn the diff is impossible to read with just one word per line. How do manpage maintainers do this?

Note that using Split diff (click the little cog menu in the Files tab) is much more readable, at least for this kind of diff. (I sometimes switch back to Unified for some code changes, but rarely.)


I agree that “bitwise NOT” should be called out for CPL, not CCF. Since the latter only affects a single bit, calling it logical or bitwise is IMO just pedantry. CPL, however, has the concept being much more load-bearing.

Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Noticed another thing—you certainly are far more thorough than I! 😅

Comment on lines -1132 to +1173
Store into
.Sy A
the bitwise OR of the value in
Bitwise OR between the value in
Copy link
Member

Choose a reason for hiding this comment

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

Oh? I'm surprised that you felt it was preferable to not call out explicitly where the result gets stored. Why's that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This mimics the wording for AND and XOR: "Bitwise OP between the argument and A."

However, you're right, it could be better. The arithmetic operations are worded as "Add the argument to A", and "Subtract the argument from A". Maybe the bitwise ones should be "Bitwise OP A with the argument".

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe, but IMO that's still a little tenuous and thus leaves the door open to confusion. I'd like the destination register pointed out explicitly.

Choose a reason for hiding this comment

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

I like the "Store into ..." wording, because it also keeps the order of the arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This affects the documentation (web-specific issues go to rgbds-www) rgbasm This affects RGBASM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve instruction documentation
3 participants