rupprecht added inline comments.
================
Comment at: lldb/source/Core/RichManglingContext.cpp:23
+RichManglingContext::~RichManglingContext() {
+ std::free(m_ipd_buf);
+ ResetCxxMethodParser();
----------------
dblaikie wrote:
> 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)
Added a comment to the header. Agree about the longer term fixes on the API
boundaries -- D100800 is a leak fix for that API, so it's clearly an API with
sharp edges.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100795/new/
https://reviews.llvm.org/D100795
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
