add Windows platform support #3
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds OS-specific file handling: Windows uses allocated memory buffers and POSIX uses mmap. Document gains a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FileOpen as File Opening Logic
participant Windows as Windows Path
participant POSIX as POSIX Path
participant Memory as Memory Management
participant Close as Close/Deallocation
rect rgba(240,248,255,0.5)
Note over FileOpen,POSIX: OS Detection & File Opening
User->>FileOpen: openWithConfig()
alt Windows System
FileOpen->>Windows: read file into allocated buffer
Windows->>Memory: allocator.alloc()/read
Memory-->>Windows: allocated buffer
Windows->>FileOpen: openFromMemoryOwnedAlloc(data_is_allocated = true)
else POSIX System
FileOpen->>POSIX: memory-map file
POSIX->>Memory: mmap()
Memory-->>POSIX: mapped region
POSIX->>FileOpen: openFromMemoryOwned(data_is_allocated = false)
end
end
rect rgba(255,240,245,0.5)
Note over Close,Memory: Deallocation Path
User->>Close: close()
alt Windows System
Close->>Memory: allocator.free(allocated buffer)
else POSIX System
alt data_is_allocated == true
Close->>Memory: allocator.free(allocated buffer)
else
Close->>Memory: munmap(mapped region)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds Windows platform support to the ZPDF library by implementing an alternative file loading mechanism. Since Windows doesn't support POSIX mmap, the PR uses allocated memory with readAll for Windows while maintaining mmap for POSIX systems.
- Adds platform detection for Windows and conditional compilation paths
- Implements allocated memory approach for Windows file loading instead of mmap
- Updates cleanup logic to handle both mmap'd and allocated memory
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return openFromMemoryOwned(allocator, data, config); | ||
| if (comptime is_windows) { | ||
| // Windows: read file into allocated memory (no mmap support) | ||
| const data = try allocator.alignedAlloc(u8, .fromByteUnits(std.heap.page_size_min), size); |
There was a problem hiding this comment.
The alignment parameter syntax appears incorrect. In Zig's alignedAlloc function, the second parameter should be a compile-time known alignment value (like std.mem.page_size), not an alignment enum. The correct syntax should be std.heap.page_size_min directly as the second parameter, not wrapped in .fromByteUnits(). This will likely cause a compilation error.
| const data = try allocator.alignedAlloc(u8, .fromByteUnits(std.heap.page_size_min), size); | |
| const data = try allocator.alignedAlloc(u8, std.heap.page_size_min, size); |
src/root.zig
Outdated
| // POSIX: check if allocated or mmap'd | ||
| if (self.data_is_allocated) { | ||
| self.allocator.free(aligned_ptr[0..self.data.len]); | ||
| } else { | ||
| std.posix.munmap(aligned_ptr[0..self.data.len]); | ||
| } |
There was a problem hiding this comment.
The cleanup logic has a potential bug. On non-Windows POSIX systems, if data_is_allocated is true, the code will call allocator.free(), but this field is only set to true in openFromMemoryOwnedAlloc which is Windows-only (lines 162, 206-226). This means the POSIX path at line 402-403 can never be taken. Either remove this unreachable code or document why POSIX might need allocated memory in the future.
| /// Whether we own the data (mmap'd or allocated) | ||
| owns_data: bool, | ||
| /// Whether data was allocated (Windows) vs mmap'd (POSIX) | ||
| data_is_allocated: bool = false, |
There was a problem hiding this comment.
The new data_is_allocated field has a default value of false, but this field is not initialized in the openFromMemory method (around line 234-246). While the default will apply, it would be clearer and more consistent with the other initialization blocks to explicitly set this field to false, especially since owns_data is already being set to false.
| if (comptime is_windows) { | ||
| // Windows: read file into allocated memory (no mmap support) | ||
| const data = try allocator.alignedAlloc(u8, .fromByteUnits(std.heap.page_size_min), size); | ||
| errdefer allocator.free(data); | ||
| const bytes_read = try file.readAll(data); | ||
| if (bytes_read != size) { | ||
| return error.UnexpectedEof; | ||
| } | ||
| return openFromMemoryOwnedAlloc(allocator, data, config); |
There was a problem hiding this comment.
The new Windows-specific file loading path (lines 154-162) lacks test coverage. The repository has comprehensive test suites (integration_test.zig, etc.), but there are no tests validating that Windows file loading and cleanup work correctly. Consider adding platform-specific tests or conditional tests that exercise this path when compiling for Windows.
|
can you take a look at the comments from copilot? |
|
Working to address now, sorry for the late response |
2 participants
I can clean up this pull request, but this enables windows builds based on my testing.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.