Fixing a mem leak which affects 3 unittests

Fixing a mem leak which affects 3 unittests.

[ivanpe.google]

.

[Mark Mentovai]

http://breakpad.appspot.com/395002/diff/1/processor/stackwalker_arm_unittest.cc
File processor/stackwalker_arm_unittest.cc (right):

http://breakpad.appspot.com/395002/diff/1/processor/stackwalker_arm_unittest....
processor/stackwalker_arm_unittest.cc:98: char *buffer =
reinterpret_cast<char*>(operator new(buffer_size));
I think it might be nicer to move the allocations into supplier, and have it
track them. I don’t like seeing different classes responsible for allocation and
deallocation. It makes it difficult to change the allocator, for one thing.

For example, I’d change the allocator here. I’d use |new char[]| paired with
|delete[]| to avoid the cast and the end-run around the type system.

[Ted Mielczarek]

http://breakpad.appspot.com/395002/diff/1/processor/stackwalker_arm_unittest.cc
File processor/stackwalker_arm_unittest.cc (right):

http://breakpad.appspot.com/395002/diff/1/processor/stackwalker_arm_unittest....
processor/stackwalker_arm_unittest.cc:98: char *buffer =
reinterpret_cast<char*>(operator new(buffer_size));
On 2012/06/08 12:10:04, Mark Mentovai wrote:
> I think it might be nicer to move the allocations into supplier, and have it
> track them. I don’t like seeing different classes responsible for allocation
and
> |delete[]| to avoid the cast and the end-run around the type system.

I agree. From the time we made that change to have the symbol supplier return
caller-owned pointers, I've wanted to have it return some sort of smart pointer
instead. That would remove a lot of the unpleasantness from this API.

[ivanpe.google]

Moved the symbol data allocation code into the symbol supplier as suggested.

Thanks,
-Ivan

[ivanpe.google]

Can you take another look?

Thanks,
-Ivan

http://breakpad.appspot.com/395002/diff/1/processor/stackwalker_arm_unittest.cc
File processor/stackwalker_arm_unittest.cc (right):

http://breakpad.appspot.com/395002/diff/1/processor/stackwalker_arm_unittest....
processor/stackwalker_arm_unittest.cc:98: char *buffer =
reinterpret_cast<char*>(operator new(buffer_size));
On 2012/06/08 12:10:04, Mark Mentovai wrote:
> I think it might be nicer to move the allocations into supplier, and have it
> track them. I don’t like seeing different classes responsible for allocation
and
> deallocation. It makes it difficult to change the allocator, for one thing.
> 
> For example, I’d change the allocator here. I’d use |new char[]| paired with
> |delete[]| to avoid the cast and the end-run around the type system.

Done.

[Mark Mentovai]

http://breakpad.appspot.com/395002/diff/4001/processor/stackwalker_unittest_u...
File processor/stackwalker_unittest_utils.h (right):

http://breakpad.appspot.com/395002/diff/4001/processor/stackwalker_unittest_u...
processor/stackwalker_unittest_utils.h:180: // Keeps a reference to the newly
allocated buffer in order to
“a reference” implies refcounting (because “a” makes it sound like there could
be more than one), but really it’s just simple new[]/delete[].

http://breakpad.appspot.com/395002/diff/4001/processor/stackwalker_unittest_u...
processor/stackwalker_unittest_utils.h:182: char*
CopySymbolDataAndOwnTheCopy(const std::string &info) {
Why not just have symbol_data_to_free be a std::vector<std::string>? Then you
don’t need any of this explicit new/delete code. You can relatively easily say
symbol_data_to_free_.push_back(info) to put a copy of info into the vector, and
then return symbol_data_to_free_.back().c_str(). Then you could get rid of the
entire destructor. But you’d want this function’s return type to change to a
const char*, which is probably all it needs to be anyway. Would that work?

[ivanpe.google]

Mark, can you take another look.

Thanks,
-Ivan

http://breakpad.appspot.com/395002/diff/4001/processor/stackwalker_unittest_u...
File processor/stackwalker_unittest_utils.h (right):

http://breakpad.appspot.com/395002/diff/4001/processor/stackwalker_unittest_u...
processor/stackwalker_unittest_utils.h:180: // Keeps a reference to the newly
allocated buffer in order to
On 2012/06/28 14:41:07, Mark Mentovai wrote:
> “a reference” implies refcounting (because “a” makes it sound like there
could
> be more than one), but really it’s just simple new[]/delete[].

Done.

http://breakpad.appspot.com/395002/diff/4001/processor/stackwalker_unittest_u...
processor/stackwalker_unittest_utils.h:182: char*
CopySymbolDataAndOwnTheCopy(const std::string &info) {
On 2012/06/28 14:41:07, Mark Mentovai wrote:
> Why not just have symbol_data_to_free be a std::vector<std::string>? Then you
> don’t need any of this explicit new/delete code. You can relatively easily say
> symbol_data_to_free_.push_back(info) to put a copy of info into the vector,
and
> then return symbol_data_to_free_.back().c_str(). Then you could get rid of the
> entire destructor. But you’d want this function’s return type to change to a
> const char*, which is probably all it needs to be anyway. Would that work?

One problem with your proposal is that c_str() returns "const char *" and what
we need here is "char *".  This can probably be addressed by modifying the
signature of SymbolSupplier::GetCStringSymbolData to accept "const char **"
instead of "char **" but such a change would cause a large chain reaction of
signature modifications which better be avoided when possible.

Another option (to avoid having the new destructor) might be to use vector of
some other type.  Unfortunately scoped_arrays cannot be used in STL containers
(no copy) and according to Google C++ coding guide other types of smart pointers
are not allowed.  I could probably create a customer wrapper around char* just
for the purpose of inserting it into the std::vector but I doubt that this will
make the code simpler.  I could definitely do that if you feel that it will be
cleaner.

Thanks,
-Ivan

[Mark Mentovai]

The point of my suggestion was to use const char* because I didn’t expect that
callers would actually need to write to the storage, but if const would ripple
through the codebase in uncontrollable ways, then this is fine for now.

LGTM.