src: cleanup uv_fs_t regardless of success or not#38996
src: cleanup uv_fs_t regardless of success or not#38996legendecas wants to merge 1 commit intonodejs:masterfrom
Conversation
|
CI: https://ci.nodejs.org/job/node-test-pull-request/38562/
|
|
CI: https://ci.nodejs.org/job/node-test-pull-request/38569/
|
| uv_fs_t req; | ||
| auto defer_req_cleanup = OnScopeLeave([&req]() { | ||
| uv_fs_req_cleanup(&req); | ||
| }); |
There was a problem hiding this comment.
An alternative approach (that may be a bit nicer here) is to use a smart-pointer type of mechanism like we use elsewhere... something like...
struct UvFsReq {
uv_fs_t req;
~UvFsReq() {
uv_fs_req_cleanup(&req);
}
}There was a problem hiding this comment.
Sounds great, I'll do an update.
There was a problem hiding this comment.
After several tries, I found that wrapping with a naive data struct did not work well in the case. We have to access members of uv_fs_t and passing the pointer of uv_fs_t back to uv, like:
uv_fs_t req;
uv_fs_open(nullptr, &req, path, O_RDONLY, 0, nullptr); // <- either we write with `&UvFsReq.req` or using operators to automatically convert (which might not be very straightforward since it implicit converting from data type to pointer type)
if (req.result < 0); // <- accessing member of `uv_fs_t`, either we write with UvFsReq.req.result or explicitly declare the member proxy in UvFsReq like `UvFsReq.result()`. This is too much for the intent, a OnScopeLeave just fit in well.
As such, I'd believe it is more readable and straightforward to use uv_fs_t here. And there are a lot of precedents in the code base.
|
Landed in e4eadb2 |
PR-URL: #38996 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
PR-URL: #38996 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
PR-URL: #38996 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
PR-URL: #38996 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
PR-URL: nodejs#38996 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
uv_fs_tregardless of success or not.uv_fs_tafter cleanup.This is a quite recent change, so I'm requesting original author @joyeecheung 's review.