Native Event Tracing IO Wrappers #1444
Conversation
improbable-prow-robot
added
jira/no-ticket
improbable-prow-robot
added
the
size/L
improbable-prow-robot
removed
the
do-not-merge/work-in-progress
Removed reference to internal namespace enum
| { | ||
| fixed (byte* fileNameBytes = ApiInterop.ToUtf8Cstr(fileName)) | ||
| { | ||
| return new IOStream(CIO.CreateFileStream(fileNameBytes, CIO.OpenMode.OpenModeDefault)); | ||
| return new IOStream(CIO.CreateFileStream(fileNameBytes, (CIO.OpenMode) openMode)); |
There was a problem hiding this comment.
I'm not sure if this kind of cast is actually valid. the C# Interop has some fancy conversion class for this reason.
Might want to double check
There was a problem hiding this comment.
It would be a compiler error if it was an invalid cast I think (the as casts fail at runtime)
…dk-for-unity into feature/native-event-tracing
There was a problem hiding this comment.
As a general comment, we should probably throw ObjectDisposedException on stream operations if the object has already been disposed
| { | ||
| fixed (byte* fileNameBytes = ApiInterop.ToUtf8Cstr(fileName)) | ||
| { | ||
| return new IOStream(CIO.CreateFileStream(fileNameBytes, CIO.OpenMode.OpenModeDefault)); | ||
| return new IOStream(CIO.CreateFileStream(fileNameBytes, (CIO.OpenMode) openMode)); |
There was a problem hiding this comment.
It would be a compiler error if it was an invalid cast I think (the as casts fail at runtime)
| var remainingCapacity = CIO.StreamGetRemainingWriteCapacityBytes(stream); | ||
| if (remainingCapacity < data.Length) | ||
| { | ||
| throw new IOException("Not enough stream capacity to write data."); |
There was a problem hiding this comment.
It may be worth have a different exception for this case as opposed to the general error case. It means users can write logic to specifically handle it.
It seems that NotSupportedException may be a good candidate as its used elsewhere in the C# standard library for 'stream full' errors (e.g. - in Memory Stream)
| public byte[] Peek(uint bytes) | ||
| { | ||
| var streamData = new byte[bytes]; | ||
|
|
||
| var bytesPeeked = 0L; | ||
| fixed (byte* streamDataPointer = streamData) | ||
| { | ||
| bytesPeeked = CIO.StreamPeek(stream, streamDataPointer, bytes); | ||
| } | ||
|
|
||
| if (bytesPeeked != -1) | ||
| { | ||
| return streamData; | ||
| } | ||
|
|
||
| var rawError = CIO.StreamGetLastError(stream); | ||
| throw new IOException(ApiInterop.FromUtf8Cstr(rawError)); | ||
| } |
There was a problem hiding this comment.
Should we make this public uint Peek(uint bytes, out byte[] bytes) to match the Read method?
There was a problem hiding this comment.
The Read method has two implementations, one where the caller provides the bytes array which is written to, and the other takes a the number of bytes to read and creates the array for them.
Should I add another implementation of Peek so that it matches Read?
| throw new IOException(ApiInterop.FromUtf8Cstr(rawError)); | ||
| } | ||
|
|
||
| public void Ignore(uint bytesToIgnore) |
There was a problem hiding this comment.
This should probably return bytesIgnored
| return CIO.StreamGetRemainingWriteCapacityBytes(stream); | ||
| } | ||
|
|
||
| private void CheckIfStreamClosed() |
There was a problem hiding this comment.
Nit: Prefer ThrowIfStreamClosed over CheckIfStreamClosed, I would assume CheckIfStreamClosed would return me a result rather than throwing an exception.
|
Please retry analysis of this Pull-Request directly on SonarCloud. |
For UTY-2685
Description
Tests
How did you test these changes prior to submitting this pull request?
What automated tests are included in this PR?
Documentation
How is this documented (for example: release note, upgrade guide, feature page, in-code documentation)?