On Fri, 2024-09-27 at 10:21 -0500, Thor Preimesberger wrote: > That's all correct. I think I got it. > > There are times where the code is emitting a json::object* that is > contained in another json object. Is it good style to return these > still as a unique_ptr?
Probably not. If you have a json::value "contained" in another json value, either as an array alement, or as an object property, then the parent "owns" the child: note how the destructor for json::object calls delete on its property values, and how the destructor for json::array calls delete on its elements. So if you have code that is creating a new json value that's about to be added somewhere in the value tree, that new json value should probably use std::unique_ptr to indicate ownership (responsibility to delete), whereas if you're borrowing a value that's already owned by something in the value tree, just use a regular pointer (or a reference, if it's guaranteed to be non-null). Hopefully that makes sense. Dave > I'm looking over what I wrote again, and in > some parts I wrap the new json object in a unique_ptr (as a return in > some function calls) and in others I use new and delete. > > Thanks, > Thor Preimesberger > > On Fri, Sep 27, 2024 at 9:18 AM David Malcolm <dmalc...@redhat.com> > wrote: > > > > On Sat, 2024-09-21 at 22:49 -0500, 4444-thor wrote: > > > From: thor <th...@protonmail.com> > > > > > > This is the second revision of: > > > > > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2024-September/662849.html > > > > > > I've incorporated the feedback given both by Richard and David - > > > I > > > didn't > > > find any memory leaks when testing in valgrind :) > > > > Thanks for the updated patch. > > > > [...snip...] > > > > > diff --git a/gcc/tree-emit-json.cc b/gcc/tree-emit-json.cc > > > new file mode 100644 > > > index 00000000000..df97069b922 > > > --- /dev/null > > > +++ b/gcc/tree-emit-json.cc > > > > [...snip...] > > > > Thanks for using std::unique_ptr, but I may have been unclear in my > > earlier email - please use it to indicate ownership of a heap- > > allocated > > pointer... > > > > > +/* Adds Identifier information to JSON object */ > > > + > > > +void > > > +identifier_node_add_json (tree t, std::unique_ptr<json::object> > > > & json_obj) > > > + { > > > + const char* buff = IDENTIFIER_POINTER (t); > > > + json_obj->set_string("id_to_locale", > > > identifier_to_locale(buff)); > > > + buff = IDENTIFIER_POINTER (t); > > > + json_obj->set_string("id_point", buff); > > > + } > > > > ...whereas here (and in many other places), the patch has a > > > > std::unique_ptr<json::object> &json_obj > > > > (expressing a reference to a a unique_ptr i.e. a non-modifiable > > non- > > null pointer to a pointer that manages the lifetime of a modifiable > > json::object) > > > > where, if I'm reading the code correctly, > > > > json::object &json_obj > > > > (expressing a non-modifiable non-null pointer to a modifiable > > json::object). > > > > would be much clearer and simpler. > > > > [...snip...] > > > > Hope the above makes sense; sorry if I'm being unclear. > > Dave > > >
