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
Factor out try/cartch duplication in nodefs code. NFC #21922
Conversation
__wasmfs_node_record_dirent(vec, name, type); | ||
stackRestore(sp); | ||
return wasmfsTry(() => { | ||
let entries = fs.readdirSync(path, { withFileTypes: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the try be only on this line? It looks like now it includes the lines after as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its possible, but it would involve complicating the code since it would looks something like this:
let entries_or_error = fwasmfsTry(() => {s.readdirSync(path, { withFileTypes: true });
if (typeof entries_or_error == number) return entries_or_error;
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that not worth it? In general I think we've preferred such scopes to be as limited as possible, to avoid the risk of unrelated errors being caught.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handler here just rethrow stuff that doesn't have a code
so it wouldn't be a big issue I think,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I don't feel strongly.
__wasmfs_node_record_dirent(vec, name, type); | ||
stackRestore(sp); | ||
return wasmfsTry(() => { | ||
let entries = fs.readdirSync(path, { withFileTypes: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I don't feel strongly.
No description provided.