Repository navigation
Fix/pinvoke review feedback - #33
Merged
nanchen2483 merged 3 commits intoOct 9, 2026
Merged
Conversation
Review feedback on Notalib#18: a cursor after the last character - where it sits while appending - came back on the last braille cell instead of after it. ToWidecharCursor clamped it to the last input character, and the result was clamped to the last output character. The clamp was not arbitrary. liblouis reports the cursor back by reading outputPos[*cursorPos] (lou_translateString.c:1389, lou_backTranslateString.c:350), so passing the end position through would read one past the array. Instead, give liblouis no cursor in that case and place it at Output.Length ourselves. The same lookup reads before the array for any negative cursor other than -1, since liblouis only treats exactly -1 as "no cursor" on the way out. Every negative cursor is now passed on as -1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review feedback on Notalib#18: lou_findTable's result was never freed, so every successful FindTable leaked a string. The rationale for leaking called it a bounded number of strings; it is one per call. It was leaked because freeing it from .NET goes to the wrong heap on Windows - .NET's string marshalling frees with CoTaskMemFree, while liblouis allocated from its C runtime: msvcrt.dll for our mingw-w64 x64/x86 builds, the UCRT for the llvm-mingw win-arm64 build. liblouis has no free function of its own. So free it with the right runtime. Off Windows that is simply libc free(), which is what NativeMemory.Free calls. On Windows, rather than infer the runtime from the RID, LiblouisHeap reads the import table of the loaded liblouis.dll and calls the free() its own calls are bound to, so it stays right whichever toolchain built the binary. A liblouis linked against a static runtime imports no free(); there the string is still left alone rather than released into the wrong heap. The InternalsVisibleTo attribute named "Nota.LibLouis.NET.Test", which no assembly is called, so the test project could not see internals. It now also names LibLouis.NET.Test, for the import table tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main converted the solution to slnx (Notalib#24), so LibLouis.NET.sln no longer exists. NativeLockTests walks up from the test output looking for it to find the library source, and now never finds it: on CI the walk runs off the top and the test fails, and in a git worktree it walks into the enclosing checkout and scans that copy's source instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
nanchen2483
approved these changes
Oct 9, 2026
This was referenced Oct 9, 2026
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.
What SHOULD have gone into PR #18, but Claude pushed to the wrong fork.