Conversation
ForNeVeR
left a comment
ForNeVeR
left a comment
There was a problem hiding this comment.
Thanks! I have some minor comments. Generally this LGTM.
Cesium.Runtime/StdIoFunctions.cs
Outdated
| catch (AccessViolationException) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
There's no point in trying to catch this. The program should fail if an exception is thrown here.
Cesium.Runtime/StdIoFunctions.cs
Outdated
| var handel = GCHandle.ToIntPtr(gch); | ||
|
|
||
| var ptr = Marshal.AllocHGlobal(sizeof(IntPtr)); | ||
| Marshal.WriteIntPtr(ptr, handel); |
There was a problem hiding this comment.
| var handel = GCHandle.ToIntPtr(gch); | |
| var ptr = Marshal.AllocHGlobal(sizeof(IntPtr)); | |
| Marshal.WriteIntPtr(ptr, handel); | |
| var handle = GCHandle.ToIntPtr(gch); | |
| var ptr = Marshal.AllocHGlobal(sizeof(IntPtr)); | |
| Marshal.WriteIntPtr(ptr, handle); |
Cesium.Runtime/StdIoFunctions.cs
Outdated
| } | ||
| } | ||
|
|
||
| internal static bool RemoveStream(IntPtr handle) |
Cesium.Runtime/StdIoFunctions.cs
Outdated
| } | ||
|
|
||
| private static unsafe void* GetLastStream() | ||
| internal static StreamHandle? GetStream(IntPtr handle) |
There was a problem hiding this comment.
Can we accept void* filePtr parameter here and perform cast inside? I think otherwise it's unnescessary noise to convert everywhere.
|
I have included your suggestions |
ForNeVeR
left a comment
ForNeVeR
left a comment
There was a problem hiding this comment.
Thanks! I believe we are getting closer to merging this.
Cesium.Runtime/StdIoFunctions.cs
Outdated
| catch (InvalidOperationException) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When does this get thrown? Looks suspicious. Maybe we shouldn't handle any exception here?
There was a problem hiding this comment.
Usually only AV occurs there. In this case, the error can occur due to an invalid pointer argument when converting to GCHandle, eventually the Target property will throw an AV if converting fail.
Cesium.Runtime/StdIoFunctions.cs
Outdated
| lock (_locker) | ||
| { | ||
| var gchAddr = Marshal.ReadIntPtr(handle); | ||
| var gch = GCHandle.FromIntPtr(gchAddr); | ||
|
|
||
| return gch.Target as StreamHandle; | ||
| } |
There was a problem hiding this comment.
Hm, now wait, why we need this _locker at all? It doesn't seem to defend any collection manipulation. Let's maybe get rid of the _locker?
There was a problem hiding this comment.
Indeed, so far the lock is not needed here
3 participants
Closes #681
GCHandles on StreamHandles are allocated on the unmanaged heap (
AllocHGlobal). A pointer to the GCHandle is used to control the StreamHandler. Locks are also added for thread-safe interaction.Keeps all standard streams (stdin, stdout, stderr) in list
Handles