lgtm
Looks generally good, but I'm a little spooked by the memory management.
Lgtm
since I didn't see anything broken; but please consider handling allocation
+
deallocation in the same class.
https://codereview.chromium.org/781443002/diff/1/src/serialize.cc
File src/serialize.cc (right):
https://codereview.chromium.org/781443002/diff/1/src/serialize.cc#newcode2313
src/serialize.cc:2313: data_ = NewArray<byte>(size_);
Related to the comment in serialize.h:
I was trying to understand how the memory management works, and the only
place I found where owns_data_ is set to true was in the superclass
constructor. So if I get this right, this piece of code relies on:
- the super-class constructor creating an 'owned' NULL pointer
- then this allocates memory
- then the destructor of the super class de-allocates it.
I guess that works, but I for one had a hard time understanding it.
[Likewise for SerializedCodeData below.]
https://codereview.chromium.org/781443002/diff/1/src/serialize.cc#newcode2408
src/serialize.cc:2408: return result;
Is this expected to the last method call on a particular
SerializedCodeData instance? In that case, maybe also null-ify the data
pointer when releasing ownership.
https://codereview.chromium.org/781443002/diff/1/src/serialize.h
File src/serialize.h (right):
https://codereview.chromium.org/781443002/diff/1/src/serialize.h#newcode411
src/serialize.h:411: SerializedData() : data_(NULL), size_(0),
owns_data_(true) {}
Here, owns_data_ == (data_ == NULL), which doesn't seem to make sense.
If I got this right, several subclass constructors will put 'owned'
data_ there by allocating memory. In that case it may make more sense to
also let those subclasses delete that memory in their destructors.
Your choice, though...
https://codereview.chromium.org/781443002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
