-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: Modernized unique_ptr construction #31654
Conversation
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.
LGTM
Just as a side note, exceptions aren’t enabled in the Node.js source tree :)
Ah okay, well my bad for that :) Didn't know that, good to know though since I'm looking to make more contributions to the project :) |
@Yuhanun It’s all good – like you said, it also makes the code more readable. 👍 |
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.
Needs @TimothyGu's suggestion added
Co-Authored-By: Timothy Gu <timothygu99@gmail.com>
Landed in 9c753b3, thanks for the PR! 🎉 |
PR-URL: #31654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
PR-URL: #31654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
PR-URL: #31654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
PR-URL: #31654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
PR-URL: #31654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
make -j4 test
(UNIX), orvcbuild test
(Windows) passesA simple first contribution.
std::unique_ptr's constructor can lead to memory leaks if the constructed value, allocated through
new
, throws an exception or something that causes an immediate backtrace on the callstack.std::make_unique prevents this by allocating the memory for you, making it safer. Not only does it make it safer, it also makes it more readable.
I'm not sure whether I followed every guideline with this, but I hope I did.