-
Notifications
You must be signed in to change notification settings - Fork 450
Bulk multibyte #4626
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
Open
SuuperW
wants to merge
3
commits into
master
Choose a base branch
from
bulk_multibyte
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Bulk multibyte #4626
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write*Rangesignatures are fine forSpan, but then why doRead*Rangeallocate and return arrays?See also #3472.
Not that you need these anyway, since the caller can just
MemoryMarshal.Castto/frombyte.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was just a lot simpler to do it that way, and the byte one returns an array IIRC so I was following that.
I knew you were wanting to update some of this stuff anyway, so I didn't think it very important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update on this: The new read methods return
IReadOnlyList, so as to match the existingReadByteRangemethod. I considered changing them all to accept aSpaninstead but that's too much work to do before release.The existing
WriteByteRangemethod was changed to accept aSpanthough, since this is good and easy to do and makes it match the new ones.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not change
IReadOnlyList<byte>toSpan<byte>inWriteByteRange's signature.The added methods are redundant, but if you're going to add them, don't use
Spanthere either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? Because it changes the API for external tools?
PLEASE, quit saying this. I have already explained to you that writing two adjacent 8-bit integers is not always equivalent to writing a 16-bit integer.
Why? This isn't even an API change, and
Spancan be more efficient.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought your comment below was in reference to cores' implementations.
Poking two adjacent 8-bit integers in virtual/logical memory is always equivalent to poking one 16-bit integer (assuming the endianness matches). If it's not, that can't be considered poking, and the core needs to be fixed.
There's a reason we're not already using
Spaneverywhere, and that's because it's not always obvious what design and signatures should be used. Which is because there are many related types, some struct and some class, each with their own quirks and limitations. I don't pretend to be an expert in them, and clearly neither are you since you used the mutableSpanhere with no intention of writing to it.As I have said before, making repeated changes to the interfaces that people use (in this case an API) is what you might call "churn" and should be avoided. Accepting a
Spanas input only to change it to e.g.ReadOnlyMemorylater is annoying. Accepting aSpanas input only to do a bunch of copying anyway is deceptive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And how would a core who's implementation differentiates between two adjacent 8-bit writes and one 16-bit write be able to make that distinction for bulk operations if the only bulk operation hands everything over as bytes? Or would you want the MemoryApi methods to call
PokeUshortin a loop? That would prevent the core from optimizing bulk operations.Not all memory addresses refer to virtual or logical memory. melonDS has addresses which map to registers, and even has some that behave more like streams than traditional RAM. (Writing 1 then 0 is not the same as writing just 0, and reading a value can cause things to happen including causing the next read to be different.) Saying "change/poke the value at this address" is meaningless because there is no value at that address. The only thing you can do is write to that address or read from it. Unless you create a new API for handling these things, the existing memory API must support it.
And all of this somehow does not apply to
ReadOnlyList? There is is somehow obvious?I disagree. There's nothing indicating an intent to write to the span. But changing it to a
ReadOnlySpanis a good idea anyway.The comment you linked does not mention "churn" and I don't see how changing keybinds is related to this. I do agree changes should be kept to a minimum, but that doesn't mean we should not use a span for the NEW methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you're describing with register-like addresses on the system bus is clearly not poking. The domain(s) with that behaviour should be
Writable: false, or maybe throw an exception with those specific addresses (not sure if there's precedence for that).