Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: InotifyEvent.name should be UncheckedArray[char] #23413

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

tdely
Copy link

@tdely tdely commented Mar 16, 2024

No description provided.

@Araq
Copy link
Member

Araq commented Mar 18, 2024

This is not correct, see https://man7.org/linux/man-pages/man7/inotify.7.html

It is an array of unspecified size at the end of the struct, but not a pointer!

@tdely
Copy link
Author

tdely commented Mar 18, 2024

You're right, but cstring is working for me; it hasn't failed yet. Can the module be used in its present state with char to get the name?

@beef331
Copy link
Collaborator

beef331 commented Mar 18, 2024

Are you actively using the name? Cause cstring and UncheckedArray[char] have vastly different semantics. For reference https://github.com/beef331/reflector/blob/master/reflector/inotifyevents.nim are more correctly Nimified bindings for Inotify.

@tdely
Copy link
Author

tdely commented Mar 18, 2024

Are you actively using the name? Cause cstring and UncheckedArray[char] have vastly different semantics. For reference https://github.com/beef331/reflector/blob/master/reflector/inotifyevents.nim are more correctly Nimified bindings for Inotify.

Yes. I need the name to add watches on created files, and remove and re-add watches when files get replaced without the original being removed or moved first. I'm happy enough with the C-style bindings, but UncheckedArray[char] as you've done works just as well as cstring.. do want me to update the PR with this change?

@beef331
Copy link
Collaborator

beef331 commented Mar 18, 2024

It should be a UncheckedArray[char] yes, the fact cstring works for you astounds me 😄

@tdely tdely changed the title fix: InotifyEvent.name should be cstring not char fix: InotifyEvent.name should be UncheckedArray[char] Mar 18, 2024
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.

None yet

3 participants