src: add BaseObjectPtr nullptr operations #56585
Conversation
Allow comparing a `BaseObjectPtr` or implicitly construct a `BaseObjectPtr` with `nullptr`.
|
Review requested:
|
nodejs-github-bot
added
c++
github-actions
bot
removed
the
request-ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56585 +/- ##
==========================================
+ Coverage 89.16% 89.19% +0.03%
==========================================
Files 662 662
Lines 191745 191797 +52
Branches 36902 36919 +17
==========================================
+ Hits 170971 171082 +111
+ Misses 13627 13562 -65
- Partials 7147 7153 +6
|
| @@ -140,7 +140,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New( | |||
| if (!(CreateV8Context(env->isolate(), object_template, snapshot_data, queue) | |||
| .ToLocal(&v8_context))) { | |||
| // Allocation failure, maximum call stack size reached, termination, etc. | |||
| return BaseObjectPtr<ContextifyContext>(); | |||
| return nullptr; | |||
There was a problem hiding this comment.
I think I'd prefer return {}; for the pattern here using the default constructor. That would match the style we've used elsewhere and, IMO makes things a bit clearer.
There was a problem hiding this comment.
IMO BaseObjectPtr is similar to a smart pointer, i.e. shared_ptr, and nullptr shall be prefered to represent a null pointer. Like what's documented here: https://github.com/nodejs/node/blob/main/src/README.md#lifetime-management
There was a problem hiding this comment.
I'd agree with @jasnell but that also extends to keeping it consistent with shared_ptr, weak_ptr and friends 🙂
There was a problem hiding this comment.
Yeah, consider it a non-blocking nit. I rather which the shared_ptr, etc cases could use the return {} approach also.
| this->~BaseObjectPtrImpl(); | ||
| return *this; |
There was a problem hiding this comment.
I think this is technically UB? I'd maybe just make this
| this->~BaseObjectPtrImpl(); | |
| return *this; | |
| return *this = BaseObjectPtrImpl(); |
so that it's more obviously correct
There was a problem hiding this comment.
Ah, right.. good catch. It looked a bit funny on review.
| @@ -140,7 +140,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New( | |||
| if (!(CreateV8Context(env->isolate(), object_template, snapshot_data, queue) | |||
| .ToLocal(&v8_context))) { | |||
| // Allocation failure, maximum call stack size reached, termination, etc. | |||
| return BaseObjectPtr<ContextifyContext>(); | |||
| return nullptr; | |||
There was a problem hiding this comment.
I'd agree with @jasnell but that also extends to keeping it consistent with shared_ptr, weak_ptr and friends 🙂
Allow comparing a
BaseObjectPtror implicitly construct aBaseObjectPtrwithnullptr.