Conversation
YoshiRulz
force-pushed
the
bulk-peek-noalloc
branch
from
f11a88d to
f78d7da
Compare
src/BizHawk.Emulation.Common/Base Implementations/MemoryDomain.cs
Outdated
Show resolved
Hide resolved
YoshiRulz
force-pushed
the
bulk-peek-noalloc
branch
from
f78d7da to
531aebb
Compare
| LogCallback($"Warning: attempted read of {srcStartOffset} outside the memory size of {d.Size}"); | ||
| return false; | ||
| } | ||
| try |
There was a problem hiding this comment.
Why does the new method have a try block?
Lua already puts everything inside a try block, and I think that is better: an exception should stop the Lua script. For C# users, I think it better to let them decide what to do.
(IMO attempting to read/write outside the range of the memory domain should also throw but that's a separate change)
| /// If this happens, the state of <paramref name="dstBuffer"/> is undefined. Anywhere from <c>0</c> bytes to all of them could have been copied. | ||
| /// The exception is when <paramref name="srcStartOffset"/> falls outside the domain, as that is checked first and results in a no-op. | ||
| /// </returns> | ||
| bool ReadByteRange(ulong srcStartOffset, Span<byte> dstBuffer, string domain = null); |
There was a problem hiding this comment.
The doc comment here is wrong. It returns false if the internal API call threw an exception, but not iff: the method itself contains a check for when the start falls outside the domain.
This is also different behavior from the other overload, which will still return some bytes just starting at the beginning of the memory domain. I think they should have the same behavior. (which again IMO should be throwing)
|
|
||
| if ((start & 1) != 0 || (end & 1) != 0) | ||
| throw new InvalidOperationException("The API contract doesn't define what to do for unaligned reads and writes!"); | ||
| if (start % 2 is not 0 || end % 2 is not 0) throw new InvalidOperationException(ERR_MSG_NOT_ALIGNED); |
There was a problem hiding this comment.
Why are you changing this? (start & 1) != 0 is easier to understand, and far nicer than start % 2 is not 0.
There was a problem hiding this comment.
Yeah & is a standard way to check individual bits.
There was a problem hiding this comment.
This isn't querying a bitfield though, it's checking address alignment. Later in the file is a % 4. Let the compiler optimise it to a mask.
There was a problem hiding this comment.
Address alignment is inherently tied to bits, which makes the bitwise operator more appropriate.
I also much prefer the standard equality operator == over is and especially prefer != over is not. (is would make sense with pattern matching, maybe even checking for null, but not for plain equality checks. And having whitespace inside the is not "operator" is just terrible imo.)
But even disregarding all that, this is a style change that isn't relevant to this PR.
| if (start % 2 is not 0 || end % 2 is not 0) throw new InvalidOperationException(ERR_MSG_NOT_ALIGNED); | ||
|
|
||
| if (values.LongLength * 2 != end - start) | ||
| if (checked((ulong) values.Length * 2UL) != addresses.Count()) |
There was a problem hiding this comment.
The checked here does nothing. Unless Length can somehow be negative, this expression can never overflow.
| } | ||
| } | ||
|
|
||
| public virtual byte[] BulkPeekByte(Range<long> addresses) |
There was a problem hiding this comment.
Would it be a good idea to make void BulkPeekByte(Range<long> addresses, byte[] values) protected, and change any call sites to use this (or the span version)?
4 participants
This isn't using
ReadOnlySpanto let the frontend and APIs read memory without copying. This still copies, but into aSpaninstead of an array, which probably makes no difference for internal use, and is slightly more convenient for ext. tools.I could have exposed the overload which takes an array reference and writes into it, but I figured this way would mitigate user error. The use of an unsigned integer for the start offset, especially.
I'd appreciate if someone could double-check my pointer arithmetic is correct, i.e. it matches the arithmetic in the other overload's implementation.
edit: Rebased, would still like that second opinion, and I think this should wait for #3965.