dblaikie added inline comments.
================
Comment at: lldb/source/Core/RichManglingContext.cpp:23
+RichManglingContext::~RichManglingContext() {
+ std::free(m_ipd_buf);
+ ResetCxxMethodParser();
----------------
JDevlieghere wrote:
> shafik wrote:
> > rupprecht wrote:
> > > JDevlieghere wrote:
> > > > Instead of managing memory by hand, can we use a `unique_ptr<char[]>`
> > > > instead?
> > > The buffer here is created by `malloc`, and from a cursory reading of
> > > `processIPDStrResult`, can be passed to other methods that call `realloc`
> > > on it. It should be paired with `free`, not `delete`. You could put that
> > > in a `unique_ptr<char, FreeDeleter>`, but when you go down that road, I
> > > think it's probably simpler to leave as-is. (You'd also have to take care
> > > to always manually `.release()` it when updating it to the realloc'd
> > > value, because you don't want to delete the pre-realloc'd buffer).
> > >
> > > (Note: this line is pulled from the original `~RichManglingContext()`
> > > definition in the header. I didn't write it so I can't defend it that
> > > well :) )
> > I didn't suggest this b/c it was clear it was a quick fix and it seemed a
> > reach to ask for that in this fix.
> Thanks for the explanation! That does sound like a bit of overkill. If it's
> not already documented, would it be useful to leave that as a comment
> somewhere?
(I think for this patch, makes sense not to try to address the
ownership/allocation model because it is non-trivial, as you've mentioned - but
longer term, it might be worth revisiting it at some point - as it has a
non-trivial & not well enforced boundary in terms of the allocation scheme
shared between RichManglingContext and ItaniumPartialDemangler. It'd be good if
that boundary were more clear rather than requiring malloc'd memory to be
passed in, and requiring the caller to free it - some kind of abstraction over
the memory ownership would probably be good to have)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100795/new/
https://reviews.llvm.org/D100795
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
