[WasmFS] Fix argument name for open in the Node backend. NFC#26597
[WasmFS] Fix argument name for open in the Node backend. NFC#26597kleisauke wants to merge 1 commit intoemscripten-core:mainfrom
Conversation
|
|
||
| // Open the file and return the underlying file descriptor. | ||
| [[nodiscard]] int _wasmfs_node_open(const char* path, const char* mode); | ||
| [[nodiscard]] int _wasmfs_node_open(const char* path, const char* flags); |
There was a problem hiding this comment.
Isn't mode the correct name for the string arguments like w+ etc? i.e. its a string which matches FILE *fdopen(int fd, const char *mode); so mode makes sense to me.
There was a problem hiding this comment.
I tried to follow Node.js conventions here:
https://nodejs.org/api/fs.html#fsopensyncpath-flags-mode
At first, I wondered "Why is mode being passed to flags?" and made this change to prevent that confusion.
There was a problem hiding this comment.
Ok.. well thats confusing. C calls the r/w/a string things mode.. not flags, but node calls is falgs. I guess maybe worth adding a comment for this.
There was a problem hiding this comment.
It's a bit confusing indeed. I think it's only a issue with fs.open{sync,}() though, as that accepts flags (which can also be a number) and mode (corresponding to mode_t in native code).
emscripten/system/lib/libc/musl/src/fcntl/open.c
Lines 5 to 16 in 8e4eee8
As possible follow-up, perhaps we could do?:
| [[nodiscard]] int _wasmfs_node_open(const char* path, const char* flags); | |
| [[nodiscard]] int _wasmfs_node_open(const char* path, int flags); |
That would make it less confusing, as it would align with POSIX open(2) (and would avoid the need of UTF8ToString()).
There was a problem hiding this comment.
... actually, we can't change const char* flags to int flags as Node.js's fs.constants are derived from the underlying file system constants, which can vary.
2 participants
Split out from #24733.