Skip to content

refactor(mmap): change from posix.mmap to Io.File.MemoryMap#126

Open
Gaweringo wants to merge 1 commit into
zigcc:mainfrom
Gaweringo:main
Open

refactor(mmap): change from posix.mmap to Io.File.MemoryMap#126
Gaweringo wants to merge 1 commit into
zigcc:mainfrom
Gaweringo:main

Conversation

@Gaweringo

Copy link
Copy Markdown

Zig has a general MemoryMap ability for files, which also supports Windows and is part of the Io interface.

I changed the mmap example to use file.createMemoryMap(). I'm not sure if the intent of the example is to specifically show the use of mmap() in Zig, or if it should show how to do memory mapping in Zig. If it's the first one, then this change could be spun out into its own recipe with a note on the mmap description about how to do memory mapping more generally in Zig. If it's the latter, then it might still be a good idea to add a callout to std.posix.mmap() dirctly.

Also, I don't know any chinese, so I was only able to change the description of the english version.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the memory mapping recipe to use the new cross-platform createMemoryMap API, enabling Windows support and removing the previous OS check. The documentation has also been updated to detail the platform-specific system calls used. Feedback suggests clearing the memory map buffer with @memset before reading to verify that data is actually read from the file, and addresses a minor typo and grammatical issue in the documentation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread assets/src/01-02.zig
Comment on lines +29 to +30
// Synchronize memory with contents of file
try mm.read(io);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure that mm.read(io) is actually reading the data from the file and not just asserting on the existing data in memory, we should clear the memory map buffer using @memset before reading.

    // Clear the memory to ensure we are actually reading from the file
    @memset(mm.memory, 0);
    // Synchronize memory with contents of file
    try mm.read(io);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh Gemini, mm.write() and mm.read() are only there to make sure memory and file are synced after they are called, this does not mean, that the file contents isn't changed before .write() is called. Doing a @memset() to 0 would overwrite the text we wrote previously making the test fail...

Comment thread src/en-US/01-02-mmap-file.smd Outdated
@jiacai2050

Copy link
Copy Markdown
Member

Thanks for your contribution. Your changes look good to me. There’s a CI failure, but it’s not caused by your modification. Would you be able to help resolve it? If you don’t have time, I can merge this PR as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants