Conversation
| ushort[] ReadU16Range(long addr, int count, string domain = null); | ||
| uint[] ReadU32Range(long addr, int count, string domain = null); | ||
| void WriteU16Range(long addr, Span<ushort> memoryblock, string domain = null); | ||
| void WriteU32Range(long addr, Span<uint> memoryblock, string domain = null); |
There was a problem hiding this comment.
Write*Range signatures are fine for Span, but then why do Read*Range allocate and return arrays?
See also #3472.
Not that you need these anyway, since the caller can just MemoryMarshal.Cast to/from byte.
There was a problem hiding this comment.
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.
| void WriteByteRange(long addr, IReadOnlyList<byte> memoryblock, string domain = null); | ||
| void WriteFloat(long addr, float value, string domain = null); | ||
|
|
||
| ushort[] ReadU16Range(long addr, int count, string domain = null); |
There was a problem hiding this comment.
Also these need to be explicit about endianness. The new Lua functions are labelled as little-endian, so put that in a doc comment at least.
There was a problem hiding this comment.
I find the whole endianness handling we have to be overly complicated, contradictory, and very confusing. Why do memory domains define an endianness, and then we don't use that defined endianness?
There was a problem hiding this comment.
The domain has an endianness property so tools like the Hex Editor can show 'words' in the expected form. I believe it's also the default when an endianness argument isn't specified in an API call.
There was a problem hiding this comment.
The memory API does not appear to care about the domain's endianness, and does not even support specifying the endianness in poke or peek calls, only with SetBigEndian, and the default endianness is little regardless of the domain. (This is just from looking at the code.)
And the Hex Editor should not have to specify the endianness that is default. IMO, a domain should only support its default endianness and it should be up to the user to swap bytes if needed. (The memory API could be such a user, but I think even that really shouldn't handle endianness because it introduces unnecessary complexity for something that would be rarely used and is trivial for users who do want it to self-implement.) ...but that's really outside the scope of this PR.
ReadU16Range should not be explicit about endianness in the call signature because ReadU16 is not explicit about endianness in the call signature. Both end up using _isBigEndian when accessing the domain.
| throw new InvalidOperationException("The API contract doesn't define what to do for unaligned reads and writes!"); | ||
| if (addresses.Count() > (uint)values.Length * sizeof(ushort)) throw new ArgumentException($"Length of {nameof(values)} must be at least {nameof(addresses)} count.", nameof(values)); | ||
|
|
||
| if (values.LongLength * 2 != end - start) |
There was a problem hiding this comment.
Hard to tell, but there are at least 2 behavioural changes in this diff: oversized buffers are now acceptable, and unaligned reads are now acceptable. The latter especially I worry about.
(If you rebase now I think the diff will be a bit cleaner.)
There was a problem hiding this comment.
Why do you worry about unaligned reads? They have defined behavior and are already supported by the single 16 and 32-bit operations.
Why would accepting oversize buffers be bad? A tool may wish to re-use a buffer for multiple reads, for efficiency. (Although I suppose since we are using a Span here instead of array the user could very easily just change the size given.)
There was a problem hiding this comment.
The behaviour around unaligned access isn't documented, and existing cores might have assumed alignment is checked.
I'm on board with allowing oversized buffers specifically for arrays when the size (here, range) is passed in, I was just highlighting the change.
There was a problem hiding this comment.
The behavior of unaligned access (even the definition of unaligned access, or if such a thing even exists) isn't documented at this level because it cannot be. It is defined by the individual cores. There are systems where unaligned access can happen internally, and we should not prohibit users from doing it manually.
If cores assume alignment is checked, they are already wrong. Alignment is never checked for non-bulk operations. And like as I said above, such an assumption would be incorrect in that is should not be assumed.
Of course, there is no way to perform unaligned access without external tools or Lua scripts. So there won't be any issues with say Hex Editor.
|
|
||
| public virtual void BulkPokeUshort(long startAddress, bool bigEndian, Span<ushort> values) | ||
| { | ||
| if (values == null) throw new ArgumentNullException(paramName: nameof(values)); |
There was a problem hiding this comment.
A ref struct cannot be null... how is this not flagged
|
Regarding all of your comments about being able to just convert between bulk byte and bulk int reads: No. The only reason I made this is to support #4555. The core MUST know what size we are reading, and the various sized reads cannot simply be re-interpreted as another. |
2 participants
To enable bulk poke/peek access for 16 and 32 bit values, I have given
MemoryDomain,IMemoryApi, andMemoryLuaLibrarysome new functions.Check if completed: