[#14] Address Copilot review follow-ups#124
Draft
SkowronskiAndrew wants to merge 1 commit into
Draft
Conversation
- CreateArchive: marshal only the first `count` entries (matching what native consumes) and validate `count` against the array lengths, instead of marshalling every element. - Clarify that the StringBuilder-output ASCII limitation is Windows-specific (Ansi marshals as UTF-8 on macOS/Linux). - NonAsciiPath.WithCopy: best-effort temp-dir cleanup so a delete failure can't mask a test failure.
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up robustness and accuracy tweaks to the UTF-8 path marshalling work in the UnityFileSystem native interop layer and its non-ASCII-path test helpers.
Changes:
- Tightens
CreateArchivemarshalling to allocate/marshal only the firstcountentries and validatescountagainst input array lengths. - Clarifies platform behavior in the
StringBuilderreturn-value marshalling comment. - Makes non-ASCII-path test temp-directory cleanup best-effort to avoid masking failures from the test body.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| UnityFileSystem/DllWrapper.cs | Adds count bounds validation and marshals only the consumed subset of string arrays; updates marshalling behavior comment. |
| UnityFileSystem.Tests/UnityFileSystemTests.cs | Wraps temp-directory deletion in a best-effort cleanup path for non-ASCII tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
198
to
+202
| // Strings returned from native (here and in the other StringBuilder-based calls) come back as | ||
| // UTF-8 bytes but are marshalled as the system ANSI code page, so only ASCII round-trips correctly. | ||
| // These are internal archive/serialized-file names, which Unity keeps ASCII, so it's not an issue in | ||
| // practice. Input paths, by contrast, are marshalled as UTF-8 (LPUTF8Str) to support non-ASCII paths. | ||
| // UTF-8 bytes but are marshalled as the system ANSI code page. On Windows that's a lossy code page, | ||
| // so only ASCII round-trips correctly (on macOS/Linux it happens to be UTF-8). These are internal | ||
| // archive/serialized-file names, which Unity keeps ASCII, so it's not an issue in practice. Input | ||
| // paths, by contrast, are marshalled as UTF-8 (LPUTF8Str) to support non-ASCII paths on all platforms. |
Comment on lines
+30
to
+33
| // Best-effort cleanup: a delete failure (e.g. a handle not yet released on Windows) | ||
| // must not mask a test failure thrown from the block above. | ||
| try { Directory.Delete(dir, true); } | ||
| catch { /* ignore */ } |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Follow-up to #122 (merged), addressing the Copilot review comments that arrived after merge. All changes are small robustness/accuracy improvements to the UTF-8 path marshalling work; no behavior change for normal use.
Changes
CreateArchive: marshal only the firstcountentries ofsourceFiles/aliases(what the native call actually consumes) instead of every array element, and validatecountagainst the array lengths up front. Previously acountlarger than the arrays could make native read past the marshalled data.GetArchiveNodecomment: clarify that the "only ASCII round-trips" limitation on theStringBuilderoutputs is Windows-specific — on macOS/Linux the Ansi marshalling is UTF-8. (These are Unity-controlled internal names that stay ASCII, so still not an issue in practice.)NonAsciiPath.WithCopy(test helper): best-effort temp-directory cleanup so aDirectory.Deletefailure (e.g. a handle not yet released on Windows) can't mask a real test failure thrown from the test body.Testing
dotnet test UnityFileSystem.Tests— non-ASCII path tests green (10/10 across versions); full build clean.