GNOME Bugzilla – Bug 777597
[RFC] Use a C++ auto pointer instead of g_autofree
Last modified: 2017-06-16 00:19:18 UTC
I discussed a few weeks ago with Cosimo about starting to use g_autoptr in GJS now that we have a dependency on GLib 2.50 for g_log_structured(), and have started to introduce it slowly. In bug 775868 Chun-wei Fan pointed out that this makes it impossible to build with MSVC, so we should stop using it. But this is C++, so we can certainly have an auto pointer that's portable across compilers! My question is should we do something like in the attached patch (which is basically the same thing as g_autoptr or std::auto_ptr) or go full C++ with std::unique_ptr (with a couple of typedefs to make it less verbose)? The latter would require a lot more code churn (C++ gurus correct me if I'm wrong, but it would require changing all functions that take Type** out-args, like gjs_string_to_utf8() for example, to take unique_ptr& instead?) One advantage is it would actually present a nice solution to bug 721678 (tl;dr - some places we g_free() memory that was allocated by SpiderMonkey's allocator which would be bad if they ever started using something that was not libc malloc().)
Created attachment 343976 [details] [review] WIP - auto pointer
Created attachment 345086 [details] [review] gi/boxed.cpp, gjs/importer.cpp: Partially use std::unique_ptr Hi, This is my bare-bones approach to using std::unique_ptr to replace the g_autoptr()/g_autofree GCCisms partially. I am not exactly that good in C++, but I think it does the job for the moment. As Philip pointer out, doing the std::unique_ptr approach would involve more since we are passing things around by reference, but I think this is the way I would want to go. With blessings, thank you!
Created attachment 345171 [details] [review] gi/boxed.cpp, gjs/importer.cpp: Partially use std::unique_ptr (take ii) Hi, As I saw in the other parts of the code (where std::unique_ptr is actually already used in the code), it would be better if we use a custom deleter anyways for all cases, so here's the new version of the patch that does the same via using classes, which wraps the use of std::unique_ptr in a pretty similar way, so to attempt to reduce verbosity. I am not attempting to convert any of those existing uses of std::unique_ptr I have two classes here, one for using g_free() for getting gchar */c-strings using GLib functions, and another for GFile/GFileEnumerator, which would then use g_object_unref(). Again, I am not that that good in C++, so this is the best I can do for now--hopefully it is not too much of verbosity. Plus, the ones that use the SpiderMonkey APIs would need a subsequent patch as there are much more changes that need to be done there, which would then need to use js_free() so that we can be safe when the underlying allocation implementation done by SpiderMonkey changes from free(). With blessings, thank you!
Created attachment 345179 [details] [review] Complete the transition to std::unique_ptr Hi, This makes the calls that use the SpiderMonkey APIs to acquire dynamically-allocated strings to use std::unique_ptr, which will use js_free() as its custom deleter, so we could also get rid of the g_autofree and g_autoptr() uses in the code. Note that this will obviously depend on the previous patch, and another wrapper class is added for such unique_ptr usage as we want to use js_free rather than g_free here (I am not sure how one would differentiate between char and gchar, as on Windows char == gchar, so hence this approach). With blessings, thank you!
Review of attachment 345171 [details] [review]: I'll need a bit more time to look over these and try out some alternatives -- sorry about that. A few initial comments that might be stupid questions: - Naming: it's a bit odd that you can have a GjsGFileAutoFree<GFileEnumerator>. - Also about naming: ideally I'd like names that are descriptive but nonetheless quick to type. How about GjsAutoUnref<T> for the g_object_unref one? - I'm not sure if std::unique_ptr has this, but how about an operator const T*() in order to avoid using get() in things like gjs_debug()? - I know composition over inheritance is usually better, but in this case it might be better to inherit? Since what we really want is a normal std::unique_ptr, just with a custom default deleter type to avoid verbosity. Would something like this work? class GjsGCharAutoFree : public std::unique_ptr<char, decltype(&g_free)> { GjsGCharAutoFree(char *ptr = nullptr) : std::unique_ptr(ptr, g_free) {} }; or even typedef std::unique_ptr<char, decltype(&g_free) = g_free> GjsGCharAutoFree; I suspect that last one is not correct syntax, but it would be cool if it worked. - For GjsJSCharAutoFree we have to be a bit more tricky, as you're supposed to use JS_free(cx, ptr) and not js_free(ptr) which is supposed to be mozjs-internal API.
Created attachment 345505 [details] [review] gi/boxed.cpp, gjs/importer.cpp: Partially use std::unique_ptr (take iii) Hi Philip, (In reply to Philip Chimento from comment #5) > Review of attachment 345171 [details] [review] [review]: > > - Naming: it's a bit odd that you can have a > GjsGFileAutoFree<GFileEnumerator>. I thought since they were the GIO's GFile APIs, hence the name, but I do agree that it could have been better, so I took GjsAutoUnref<T> > > - I'm not sure if std::unique_ptr has this, but how about an operator const > T*() in order to avoid using get() in things like gjs_debug()? Hmm, we could, but not all the time, unless we use const_cast in those cases, otherwise the code will fail to build due to type conversion issues, like "C2664: Cannot convert from const char* to char*". So, we have the overloaded operator*() for GjsAutoGChar (for gchar) and for GjsAutoJSChar (for char* obtained using SpiderMonkey APIs), but retained using the .get() method for the GjsAutoUnref case (all cases) and the GjsAutoJSChar case (few cases) where using the operator would not work due to the issue above. > Would something like this work? > > class GjsGCharAutoFree : public std::unique_ptr<char, decltype(&g_free)> > { > GjsGCharAutoFree(char *ptr = nullptr) : std::unique_ptr(ptr, g_free) > {} > }; > > or even > > typedef std::unique_ptr<char, decltype(&g_free) = g_free> > GjsGCharAutoFree; > Unfortunately neither worked for me, so I kept the class declaration in the same format. With blessings, thank you!
Created attachment 345506 [details] [review] Complete the transition to std::unique_ptr (take ii) Hi Philip, > - For GjsJSCharAutoFree we have to be a bit more tricky, as you're supposed > to use JS_free(cx, ptr) and not js_free(ptr) which is supposed to be > mozjs-internal API. I think I managed to do so with JS_free(), after doing some research in the issue, by setting up a helper class. Since we are having JSContext's in the places where we make use of GjsAutoJSChar, this was made easier. I did, however, require the .reset() method (in this patch and the previous patch) to pass in a parameter as we need to use a default one with no arguments when we allocate an array using new(), then construct them during the for loop, which is shown in the patch, and to avoid ambiguity issues, which causes the compiler to complain. Visual Studio is quite picky about types, which for some regards I think is a good thing. With blessings, thank you!
Review of attachment 345505 [details] [review]: Nice work, thanks. I think the autoptr classes still need a bit of tweaking. My ideal is to be able to use them transparently, using operators to make them as much like regular pointers as possible. That will have the added bonus of also making the patches smaller, and easier to spot any mistakes while converting the code base to use them. I've played around with it a bit here, and come up with the following. I think I've solved the casting problem by providing operators; at least, this compiles warning-free on GCC 6.3.1. class GjsAutoGChar : public std::unique_ptr<char, decltype(&g_free)> { public: typedef std::unique_ptr<char, decltype(&g_free)> U; GjsAutoGChar(char *str = nullptr) : U(str, g_free) {} operator const char *() { return get(); } }; template <typename T> class GjsAutoUnref : public std::unique_ptr<T, decltype(&g_object_unref)> { public: typedef std::unique_ptr<T, decltype(&g_object_unref)> U; GjsAutoUnref(T *ptr = nullptr) : U(ptr, g_object_unref) {} operator T *() { return U::get(); } }; Now I am by no means experienced with C++, so I may be overlooking something here, but it seems to me that the more we can let std::unique_ptr deal with copy and move semantics, the better off we are. One more thing, what do you think of removing the "G" and making it GjsAutoChar? It's one less character and also fits with the original GJS style of using char* rather than gchar*. ::: gjs/importer.cpp @@ +71,3 @@ return false; g_autofree char *path = NULL; You could move this inside the else clause at line 79 now. ::: gjs/jsapi-util.h @@ +33,3 @@ #include "gi/gtype.h" +#include <memory> Not all files adhere to this yet, but I'm trying to standardize on this order: - standard library headers - GNOME platform headers - local headers So please move this before <stdbool.h>.
Review of attachment 345506 [details] [review]: ::: gi/arg.cpp @@ +422,3 @@ if (!gjs_string_to_##lctype(cx, str_val, &cstr)) \ return false; \ + *pointer_out = g_strdup(*cstr); \ This seems wrong to me, as the string should not have to be copied. I think we need something like the std::unique_ptr::release() method here. @@ +594,3 @@ void **arr_p) { + GjsAutoJSChar *result = new GjsAutoJSChar[length+1]; I guess we should prefer standard library containers when working with C++ objects. @@ -2572,2 @@ out: - if (keyutf8) g_free(keyutf8); When converting the last cleanup action after an "out:" label to RAII, what I've been doing so far is just getting rid of the "goto out"s in the function altogether. ::: gi/value.cpp @@ +386,3 @@ return false; + g_value_take_string(gvalue, g_strdup(*utf8_string)); At first I thought this should not need a copy either, but ... actually it does, because we're going from a JS_free string to a g_free string. There are a few things we could do here: - Use g_value_dup_string() instead. - Implement zero-copy conversion between GjsAutoGChar and GjsAutoJSChar, and then we could change that to automatically copy if SpiderMonkey changed their allocator. ::: gjs/jsapi-util-args.h @@ +105,3 @@ throw g_strdup_printf("Wrong type for %c, got char**", c); } + *ref = g_strdup(*tmp_ref); This one should use release(). Although, maybe there's some more elegant way to change this template to use GjsAutoJSChar in the first place... ::: gjs/jsapi-util.h @@ +66,3 @@ }; +class GjsJSFreeArgs { Yes, I like this idea of making a deleter functor. @@ +74,3 @@ + } +private: + JSContext *m_cx; I have a slight preference for putting the private members at the top of a class. @@ +79,3 @@ +class GjsAutoJSChar { +public: + GjsAutoJSChar(JSContext *cx = nullptr, char *str = nullptr) I would suggest removing the nullptr default from cx. It should be illegal to create GjsAutoJSChar with a null context, I think. @@ +438,3 @@ bool gjs_string_to_utf8 (JSContext *context, const JS::Value string_val, + GjsAutoJSChar *utf8_string_p); Again I'm not the most experienced C++ programmer so I may be wrong about this, but I think the usual C++ idiom would be to use a reference for an in-out param: GjsAutoJSChar&
(In reply to Philip Chimento from comment #9) > Review of attachment 345506 [details] [review] [review]: > @@ +438,3 @@ > bool gjs_string_to_utf8 (JSContext *context, > const JS::Value string_val, > + GjsAutoJSChar > *utf8_string_p); > > Again I'm not the most experienced C++ programmer so I may be wrong about > this, but I think the usual C++ idiom would be to use a reference for an > in-out param: GjsAutoJSChar& Aha, I see why you used the pointer though. So that you could pass a GjsAutoJSChar to the function the same way that a regular pointer would work. Not sure which I like better, but I'm leaning towards the reference.
Created attachment 345597 [details] [review] Here's the patch that I came up with while playing around with this yesterday. What do you think?
Review of attachment 345597 [details] [review]: Hi Philip, I think your version of the patch looks nicer than mine:), but a few points that I noticed: With blessings, thank you! ::: gjs/importer.cpp @@ +77,2 @@ } else { + g_autofree char *path = NULL; I needed to avoid the g_autofree to test the patch, but since this is a partial attempt (and we need to fixup this in the later patch as we are calling by reference using SpiderMonkey APIs), I think I can live with that one when this goes into master and temporarily patch it myself to avoid this. ::: gjs/jsapi-util.h @@ +42,3 @@ + operator const char *() { + return get(); + } I think we need to overload operator= as well, as otherwise Visual Studio 2013 will fail due to C2280 (attempting to reference a deleted function), as we try to assign the new gchar* value. I did something like this to fix the build, by adding to the class definition: void operator= (const char* str) { reset(const_cast<gchar*>(str)); } -or- void operator= (char* str) { reset(str); }
Created attachment 345603 [details] [review] gi/boxed.cpp, gjs/importer.cpp: Partially use std::unique_ptr (take iv) Hi Philip, Here's the updated patch to get rid of g_autofree and g_autoptr() for gi/boxed.cpp and gjs/importer.cpp for the ones that use the GLib/GIO APIs to allocate the returned strings (i.e., the first patch). This -Incorporates your suggested patch, with the overloaded operator=. -Does GjsAutoGChar->GjsAutoChar. With blessings, thank you!
Review of attachment 345603 [details] [review]: This looks fine except I don't yet understand why the explicit operator= is necessary. I'm sorry but we might be getting into a bit of a rebase race when bug 777962 gets merged... what's your timeline for getting this merged? Are you trying hard to get it into GNOME 3.24 or can we be more relaxed? ::: gjs/jsapi-util.h @@ +27,2 @@ #include <stdbool.h> +#include <memory> Sorry to nitpick ... but please place this before stdbool.h so it's alphabetical. @@ +45,3 @@ + + void operator= (const char* str) { + reset(const_cast<gchar*>(str)); gchar -> char I don't quite understand why this is necessary, maybe I need to think about it some more... so it seems VS2013 says this operator is deleted by default, but GCC doesn't, is that correct?
Hi Philip, Thanks for letting me know. (In reply to Philip Chimento from comment #14) > Review of attachment 345603 [details] [review] [review]: > > This looks fine except I don't yet understand why the explicit operator= is > necessary. > > I'm sorry but we might be getting into a bit of a rebase race when bug > 777962 gets merged... what's your timeline for getting this merged? Are you > trying hard to get it into GNOME 3.24 or can we be more relaxed? I am thinking we could even get rid of the stdbool.h include since the bool type is included in standard C++, at least for C++-11 that we use, IMHO, but maybe do that in another bug/patch series. I do hope to get it merged by 3.24, as we have the Windows build infrastructure in place. I think I will need to work on the mozjs-38 branch as well for this, as a result. Let me know what is the best for you--if getting this by 3.24 is too much of a rush, I think we can do it the next cycle, but it would be the best not to ship the Visual Studio build items for 3.24 then, as they won't work due to the GCCisms from g_autoptr()/g_autofree that this patch series is attempting to address. The code by itself, I think, can work Windows via MinGW/mingw-w64 builds if one can get SpiderMonkey to build with it (which, I am not sure and am unable to try), sans the x64 issue mentioned in the other bug. > I don't quite understand why this is necessary, maybe I need to think about > it some more... so it seems VS2013 says this operator is deleted by default, > but GCC doesn't, is that correct? Well, from what I see, this is what MSDN[1] says, though this is for Visual Studio 2015, but I suspect that this is the same thing on 2013 and most likely the upcoming 2017: There is a line that says the following: "When you see error C2280 in connection with a unique_ptr, it is almost certainly because you are attempting to invoke its copy constructor, which is a deleted function." So it seems to me the default operator= won't work as it will trigger the copy constructor, so I had to overload it. I will get the new patch up here with the niticks fixed ASAP--the other one I need to look more into as there are crashes that need to be addressed for the enhanced unique_ptr transition. [1]: https://msdn.microsoft.com/en-us/library/mt771517.aspx
Created attachment 345723 [details] [review] gi/boxed.cpp, gjs/importer.cpp: Partially use std::unique_ptr (take v) Hi Philip, Here's the new patch with the nitpicks addressed. With blessings, thank you!
(In reply to Fan, Chun-wei from comment #15) > Hi Philip, > > Thanks for letting me know. > > (In reply to Philip Chimento from comment #14) > > Review of attachment 345603 [details] [review] [review] [review]: > > > > This looks fine except I don't yet understand why the explicit operator= is > > necessary. > > > > I'm sorry but we might be getting into a bit of a rebase race when bug > > 777962 gets merged... what's your timeline for getting this merged? Are you > > trying hard to get it into GNOME 3.24 or can we be more relaxed? > > I am thinking we could even get rid of the stdbool.h include since the bool > type is included in standard C++, at least for C++-11 that we use, IMHO, but > maybe do that in another bug/patch series. Agreed. > I do hope to get it merged by 3.24, as we have the Windows build > infrastructure in place. I think I will need to work on the mozjs-38 branch > as well for this, as a result. Let me know what is the best for you--if > getting this by 3.24 is too much of a rush, I think we can do it the next > cycle, but it would be the best not to ship the Visual Studio build items > for 3.24 then, as they won't work due to the GCCisms from > g_autoptr()/g_autofree that this patch series is attempting to address. I think we can merge the first patch in time for 1.47.90 and the GNOME 3.24 feature freeze, which I hope to release today. Unfortunately it will still contain some more g_autofree because I've been using it in the mozjs38 branch. Since replacing those new uses of g_autofree with the auto classes is a small bugfix and unlikely to regress, I think that would be OK to commit in the soft freeze period. And I will use the auto classes from now on. The second, more comprehensive, patch might be a bit too much for even the soft freeze period. But on the other hand I guess I would be OK with a temporary patch to go back to using manual g_free() on strings that are passed as out parameters to functions like gjs_string_to_utf8(), so that we wouldn't have to change the function signatures to take the unique_ptrs, if it will allow you to build 1.48 officially on Windows. I think that such a patch would also be OK to commit in the soft freeze period. The soft freeze changes to a hard freeze on March 13, which would be the release of GJS 1.47.92. I think that's enough time to come up with those two patches, what do you think?
Review of attachment 345723 [details] [review]: +1
Created attachment 345809 [details] [review] Temprary patch to avoid g_autofree for GNOME 3.24 Hi Philip, Here's the quick patch to avoid g_autofree for GNOME 3.24 by using the traditional g_free() approach, so that we can try to move the other items to smart pointers in the next dev cycle. Also, we need to include <tuple> in gi/object.cpp as compilers such as Visual Studio can be quite specific about the things that are included in the std namespace, meaning we need to include those headers that we get the needed functionality specifically. p.s. Something I noticed, hopefully it is not on my side, the gjs-console program does not terminate when I close the window that is created by running gjs-console.exe examples\gtk.js, which was not the case when the code was running against SpiderMonkey 31. Is this expected for now? With blessings, thank you!
Review of attachment 345809 [details] [review]: Looks good, feel free to commit it after fixing the few comments. Thank you! ::: gi/ns.cpp @@ +63,3 @@ if (!gjs_get_string_id(context, id, &name)) { *resolved = false; + g_free(name); name should not need freeing here, since it won't be filled in if gjs_get_string_id() returns false. @@ +71,3 @@ strcmp(name, "toString") == 0) { *resolved = false; + g_free(name); Use "goto out" here and in the rest of the function for consistency? (Or does that jump across initialization boundaries? I can't tell for sure in the diff view.) ::: gjs/importer.cpp @@ +142,3 @@ + char *parent_path = NULL; + if (!gjs_string_to_utf8(context, parent_module_path, &parent_path)) { + g_free(parent_path); This shouldn't need freeing if gjs_string_to_utf8() returns false. @@ +291,3 @@ + char *path = NULL; + if (!gjs_string_to_utf8(cx, module_path, &path)) { + g_free(path); Ditto. @@ +808,3 @@ + if (!gjs_get_string_id(context, id, &name)) { + g_free(name); Ditto.
Although the patch has a few compile errors, all similar to the one below: gjs/gi/fundamental.cpp: In function ‘bool fundamental_instance_resolve(JSContext*, JS::HandleObject, JS::HandleId, bool*)’: gjs/gi/fundamental.cpp:384:1: error: jump to label ‘out’ [-fpermissive] out: ^~~ gjs/gi/fundamental.cpp:330:14: note: from here goto out; ^~~ gjs/gi/fundamental.cpp:334:18: note: crosses initialization of ‘Fundamental* proto_priv’ Fundamental *proto_priv = (Fundamental *) priv; ^~~~~~~~~~ Please do make sure it builds locally before committing it. (If these aren't warnings or errors in VS, then maybe use the Travis CI build to check it or get me to compile it before committing.)
Hi Philip, Hmm, seems like we are getting similar errors but in different parts of the code... (the "consistency" part you mentioned had something to do with this, as Visual Studio fails to build otherwise with some initialization skipped-related error in other parts of the code)... I will get to a new patch later today, and will test build things on my Ubuntu system as well to double check things so that things compile on both sides. Perhaps the best way would be to use a function that deals with freeing things up and returning the bool's as needed. Thanks for letting me know, with blessings, thank you!
Created attachment 345938 [details] [review] Temprary patch to avoid g_autofree for GNOME 3.24 (take ii) Hi, This is the updated temporary patch for GNOME 3.24--I tested this also on Ubuntu 16.10 in addition to Visual Studio 2013. Sorry for any inconvenience caused. With blessings, and cheers!
Review of attachment 345938 [details] [review]: Sorry, but I'm not much in favour of using gjs_return_bool_and_free() everywhere. It seems like a lot of churn and reduced readability for something that will hopefully be replaced next cycle. Rather, let's just take the previous version of the patch, and add g_free(something); return some_bool; where necessary in order to avoid crossing initialization boundaries. Or alternatively, move the initialization up to the top of the scope in those cases. I apologize for asking for yet another revision! But it's getting close. Thank you for doing this work.
Created attachment 346048 [details] [review] Temprary patch to avoid g_autofree for GNOME 3.24 (take iii) Hi Philip, Ok, here's the patch that does it in a more conservative way, i.e, using g_free whenever we are going to return after we acquire the items we need on the heap. I think it gets too troublesome trying to track where different compilers error out due to skipped initializations, and I think it is better consistency-wise (plus, when we do switch to smart pointers, the compiler can help us out where we need to get rid of the g_free()'s for the items we get using SpiderMonkey's APIs on the heap). Thanks for the suggestions along the way. With blessings, and cheers!
Review of attachment 346048 [details] [review]: OK, this seems like a good alternative.
Review of attachment 346048 [details] [review]: Unfortunately though, it is causing some tests to fail. I can't spot the error, but I'll upload my test log to the bug.
Created attachment 346120 [details] Log with failing tests
Created attachment 346124 [details] [review] Temprary patch to avoid g_autofree for GNOME 3.24 (take iv) Hi Philip, Oops, it seems that I g_free()'ed some stuff prematurely. Sorry! With blessings, thank you!
Review of attachment 346124 [details] [review]: +1, works fine for me now. I'm a bit embarrassed that I didn't spot the mistake last time. It's always good for me to remember why tests are important :-)
Hi, are you interested in picking this up again? Now that we are early in the 3.26 cycle, this would be a good time to get it landed. The 1.49.1 release is in one week, so it would be especially nice to land it before then.
Created attachment 350078 [details] [review] Dist gjs/jsapi-class.h Hi, This file needs to be dist'ed as the build will rely on it (and so will fix 'make distcheck'). With blessings, thank you!
Created attachment 350086 [details] [review] Complete the transition to std::unique_ptr (take iii) Hi, Here we go, coming back to this... This shifts from the temporary approach we used for GNOME-3.24 to use std::unique_ptr. This is done in a manner that the items where we used to use g_autofree/g_autoptr is completely replaced by this, in a conservative manner. With blessings, thank you!
Review of attachment 350078 [details] [review]: +1, my mistake.
Thanks for following up with the patch! I appreciate it and I'm looking forward to be able to use it in new code. I'll try to review it in detail soon, but here are a few comments at first glance: - I'd suggest that get() returns const char * so that it's clear that the autoptr still owns the string. - There are a few cases where we have to call release() and assign the string to a pointer in some struct, for example. Should we have release() copy the string in that case, since after the autoptr goes out of scope the string is no longer associated with a context that you can call JS_free() with? My guess is that's not necessary, but it's worth thinking about. - To me, the operator* feels a bit odd. Is it possible to have an operator const char * so that you can use the autoptr transparently in functions like strlen() and strcmp()?
Hi Philip, (In reply to Philip Chimento from comment #37) > - I'd suggest that get() returns const char * so that it's clear that the > autoptr still owns the string. Unfortunately, I don't think we can. There are functions that use this .get() thing for a reason, as they won't accept const char*, such as object_instance_resolve_no_info(). > - There are a few cases where we have to call release() and assign the > string to a pointer in some struct, for example. Should we have release() > copy the string in that case, since after the autoptr goes out of scope the > string is no longer associated with a context that you can call JS_free() > with? My guess is that's not necessary, but it's worth thinking about. We think we could use g_strdup(m_ptr.get()) if we want, and so that we can continue using g_free() as are probably now doing. However, from what I understand from std::unique_ptr::release(), it passes the ownership of the allocated memory to the pointer that we assign the return value of std::unique_ptr::release() to, and so we become responsible for freeing the pointer ourselves. Please see: http://stackoverflow.com/questions/25609457/unique-ptr-explicit-delete > > - To me, the operator* feels a bit odd. Is it possible to have an operator > const char * so that you can use the autoptr transparently in functions like > strlen() and strcmp()? I will try to look into this a bit later. Battery running out :(. Thanks for the notes! With blessings, thank you!
(In reply to Fan, Chun-wei from comment #38) > Hi Philip, > > (In reply to Philip Chimento from comment #37) > > > - I'd suggest that get() returns const char * so that it's clear that the > > autoptr still owns the string. > > Unfortunately, I don't think we can. There are functions that use this > .get() thing for a reason, as they won't accept const char*, such as > object_instance_resolve_no_info(). It would be fine to change those functions to take const char*. (I checked object_instance_resolve_no_info() and it does not need to modify the string.) If they are not modifying the string, then they should accept const char*. If they are modifying the string, then it should be an error to pass the string owned by the autoptr. > > - There are a few cases where we have to call release() and assign the > > string to a pointer in some struct, for example. Should we have release() > > copy the string in that case, since after the autoptr goes out of scope the > > string is no longer associated with a context that you can call JS_free() > > with? My guess is that's not necessary, but it's worth thinking about. > > We think we could use g_strdup(m_ptr.get()) if we want, and so that we can > continue using g_free() as are probably now doing. However, from what I > understand from std::unique_ptr::release(), it passes the ownership of the > allocated memory to the pointer that we assign the return value of > std::unique_ptr::release() to, and so we become responsible for freeing the > pointer ourselves. > > Please see: > http://stackoverflow.com/questions/25609457/unique-ptr-explicit-delete Indeed, after calling release() you have to free the pointer yourself. But sometimes you have to give some other struct ownership of the pointer, and defer the freeing until later. For example when we put it into arg->v_pointer in gjs_value_to_g_argument() in arg.cpp. In that case, the later code will just free it with g_free(), instead of JS_free(cx, ...) because we effectively de-associate it from GjsAutoJSChar's deleter. That will work fine as long as the malloc implementations are the same, but it may not always be the case. See also bug 721678.
Review of attachment 350086 [details] [review]: Thanks very much for the patch! I was able to take a more detailed look, pending the comments I made the other day. I noticed it still needs some conversion to the new API in test/gjs-tests.cpp. ::: gi/arg.cpp @@ +639,3 @@ { GITypeTag element_type; + GjsAutoJSChar result(context); May as well move this inside the if-block, since it's not used outside. @@ +648,3 @@ return false; + *arr_p = result.release(); + *length = strlen((const char*)*arr_p); Either use a C++-style cast, or move this up one line so you can say *length = strlen(result.get()). @@ +2565,3 @@ goto out; + keyutf8.reset(nullptr); To avoid this, you can move the declaration of keyutf8 inside the for loop. That way, it will be freed at the end of the loop even without this. @@ +2570,3 @@ result = true; out: In general: when your autoptr refactors make it possible to remove the "goto out" pattern, let's do that :-) ::: gi/boxed.cpp @@ +136,1 @@ + if (priv == NULL) Pre-existing defect: Since you're touching this code anyway, change it to if (!priv) or if (priv == nullptr) to fit with the style guide. (Same comment in several places) ::: gi/object.cpp @@ +404,3 @@ if (priv == NULL) { /* If we reach this point, either object_instance_new_resolve + * did not throw (so *name == "_init"), or the property actually Thanks for paying attention to the comments too :-) ::: gjs/byteArray.cpp @@ +426,2 @@ encoding_is_utf8 = true; + encoding.reset(nullptr); I think you can just leave this statement out, and let encoding be freed when it goes out of scope. (Same comment on line 565 below.) @@ +583,2 @@ g_byte_array_set_size(priv->array, 0); + g_byte_array_append(priv->array, (guint8*) *utf8, strlen(*utf8)); Pre-existing defect: As long as you're touching this code, change the C-style cast to a C++-style cast. ::: gjs/context.cpp @@ +119,3 @@ JS::Value *vp) { + GjsAutoJSChar s(context); As an optimization, move this down to just before it is needed, before any early returns. @@ +190,3 @@ { GString *str; + GjsAutoJSChar s(context); Move this one inside the if (jstr != NULL) block. ::: gjs/coverage.cpp @@ +1021,3 @@ } + return g_bytes_new_take((guint8 *) *statistics_as_json_string, I am pretty sure you have to release the autoptr here. This seems to be causing a double-free crash in the tests. ::: gjs/importer.cpp @@ +500,3 @@ char *filename; char *full_path; + GjsAutoJSChar dirname(context); Consider moving this inside the loop so you don't have to reset it. ::: gjs/jsapi-util-args.h @@ +90,3 @@ char **ref) { + GjsAutoJSChar tmp_ref(cx); Move this down below the nullable case's early return, after the } on line 97. ::: gjs/jsapi-util.cpp @@ +621,3 @@ JS::RootedValue js_lineNumber(context), js_fileName(context); unsigned lineNumber; + GjsAutoJSChar utf8_fileName(context); Pre-existing defect: As long as you're touching this code, go ahead and rename it utf8_filename to conform with the style guide. @@ +631,3 @@ gjs_string_to_utf8(context, js_fileName, &utf8_fileName); else + utf8_fileName.reset("unknown"); This would try to JS_free the string literal, so it has to be reset(JS_strdup(context, "unknown")) I think. ::: gjs/jsapi-util.h @@ +61,3 @@ }; + Code style nitpick: only one blank line needed here. @@ +88,3 @@ + return m_ptr.release(); + } + void reset(char *str) { Code style nitpick: indent with spaces. @@ +95,3 @@ + } +private: + std::unique_ptr<gchar, GjsJSFreeArgs> m_ptr; Code style nitpick: put private first when possible. Also, use char instead of gchar. @@ +377,3 @@ + const char *description, + JS::HandleId property_name, + char **value); What changed here?
Created attachment 350960 [details] [review] Complete the transition to std::unique_ptr (take iv) Hi Philip, Sorry, it took a while since I got back to this bug, as I wanted to ensure that 781525 gets fixed properly, as it had a bigger issue with people. Sorry about that! This is the new patch for using std::unique_ptr--I hope I understood your comments correctly while doing this patch, and also fixed the build of test/gjs-tests.cpp (which, I couldn't build the gjs-tests test program on Windows as there are some items in the test program code that must be ported for Windows, and when it does get ported to Windows, we will require Visual Studio 2015 or later since some more C++11 items must be supported in the test code, and AFAIK SpiderMonkey 38 does not support building on Visual Studio 2015, but I think 45 does). I am not that sure about how the log() in the JS scripts gets directed in our code, but one thing I noticed is that when I run the gtk.js example, the "JS LOG" messages (which display on exit) display correctly on 32-bit Windows builds but displays gibberish on 64-bit Windows builds (although the texts display properly on the UI of the window that pops up). I hope I did not miss anything in the process, since this popped up after this patch, but this is what I have for now. Thanks a bunch along the way! With blessings, thank you!
Thanks for the patch! This looks really good. I didn't get all the way through reviewing it but I'll try to get back to you tomorrow. I think I've figured out the JS LOG gibberish issue though; apparently clang is a bit stricter in this regard, so it is catching some extra things. The issue is that when you are doing something with a varargs function like GjsAutoJSChar s(cx); ... g_message("JS LOG: %s", s); the compiler can't tell that it needs to invoke the (const char *) operator to convert it to a string. In this case it needs to be s.get(). It's a bit unnatural to remember so maybe you have a better idea for a more natural syntax? Also, strange that it works on 32-bit!
Review of attachment 350960 [details] [review]: All right, lots of comments, but don't get discouraged; most of the comments are only marking where you have to add .get() for a varargs function. I think the patch is looking great and I'm really happy with the API that you've landed on. I was thinking a bit more about the varargs issue. It's annoying to have to add .get() everywhere, as you could tell from my comments on the previous patch I was hoping we could avoid that. But I think there's probably no way around it for varargs functions. One possibility would be to use typesafe C++ formatting (std::cerr << ... instead of g_printerr, std::stringstream objects to pass to debug functions) but I think that's actually more code churn and doesn't give much benefit. ::: gi/arg.cpp @@ +1487,2 @@ if (gjs_string_to_utf8(context, value, &utf8_str)) // doing this as a separate step to avoid type-punning This comment can be deleted now. @@ +2533,2 @@ GArgument keyarg, valarg; bool result; This line can also be removed. @@ +2552,3 @@ while (g_hash_table_iter_next (&iter, &keyarg.v_pointer, &valarg.v_pointer)) { + GjsAutoJSChar keyutf8(context); Move this declaration down just before it's needed, to avoid constructing/destructing in case of early return. ::: gi/boxed.cpp @@ +288,3 @@ if (field_info == NULL) { gjs_throw(context, "No field %s on boxed type %s", name, g_base_info_get_name((GIBaseInfo *)priv->info)); This has to be name.get(), I think, since the compiler can't guess that it needs the (const char *) operator because of the varargs function. ::: gi/object.cpp @@ +866,1 @@ name, g_type_name(gtype)); This has to be name.get() as well. (And if you're touching this line, may as well fix the alignment :-) @@ +1610,2 @@ gjs_throw(context, "No signal '%s' on object '%s'", signal_name, Also needs a .get() @@ +1698,2 @@ gjs_throw(context, "No signal '%s' on object '%s'", signal_name, Also needs a .get() @@ +1709,1 @@ g_type_name(G_OBJECT_TYPE(priv->gobj)), The signal_name in the line above this needs a .get() too ::: gjs/byteArray.cpp @@ +424,2 @@ */ if (strcmp(encoding, "UTF-8") == 0) { optional: I think you could simplify this even more with encoding_is_utf8 = (strcmp(encoding, "UTF-8") == 0); since we don't have to care about freeing and resetting the string now. @@ +561,2 @@ */ if (strcmp(encoding, "UTF-8") == 0) { optional: Same simplification here. ::: gjs/context.cpp @@ +147,1 @@ g_message("JS LOG: %s", s); Needs to be s.get() ::: gjs/coverage.cpp @@ +494,3 @@ static void init_covered_function(GjsCoverageFunction *function, + const char *key, I was wondering about this, but I think copying the key is indeed the right thing to do here. An alternative would be to make GjsCoverageFunction use a GjsAutoJSChar internally, but I think that would make things needlessly complicated. @@ +1022,3 @@ + int json_string_len = strlen(statistics_as_json_string); + guint8 *json_string_guint8 = nitpick: I tend to use "auto" when the type is already clear from a cast, so that the type doesn't have to be repeated. Also, avoid repeating the type in the variable name: call it something like "json_bytes". @@ +1023,3 @@ + int json_string_len = strlen(statistics_as_json_string); + guint8 *json_string_guint8 = + reinterpret_cast<guint8*>(statistics_as_json_string.release()); nitpick: Prefer uint8_t instead of guint8. @@ +1370,3 @@ } g_message("JS COVERAGE MESSAGE: %s", s); Needs to be s.get() ::: gjs/importer.cpp @@ +87,1 @@ } The line above this needs to have path.get() @@ +142,3 @@ if (!gjs_string_to_utf8(context, parent_module_path, &parent_path)) return false; module_path_buf = g_strdup_printf("%s.%s", parent_path, module_name); Needs to be parent_path.get() @@ +288,3 @@ if (!gjs_string_to_utf8(cx, module_path, &path)) return false; GjsAutoChar output = g_strdup_printf("[GjsModule %s]", path); Needs to be path.get() @@ +542,2 @@ for (i = 0; i < search_path_len; ++i) { + GjsAutoJSChar dirname(context); Line 614 needs to have dirname.get() ::: gjs/jsapi-util.cpp @@ +220,2 @@ else gjs_throw(cx, "No property '%s' in object %p (or %s)", name, Needs to be name.get() (also on line 218) @@ +569,3 @@ if (exc_str != NULL) gjs_string_to_utf8(cx, JS::StringValue(exc_str), &utf8_exception); + return utf8_exception.release(); Instead of releasing here, I'd change the return type of this function to GjsAutoJSChar. Since release() actually copies, you could avoid the copy that way. @@ +638,2 @@ g_critical("JS ERROR: %s: %s @ %s:%u", utf8_message, utf8_exception, + utf8_filename, lineNumber); utf8_message.get() and utf8_filename.get() @@ +640,3 @@ } else { g_critical("JS ERROR: %s @ %s:%u", utf8_exception, + utf8_filename, lineNumber); utf8_filename.get() @@ +658,1 @@ + if (message != NULL) { nitpick: Prefer C++-style "if (message)" or "if (message != nullptr)" @@ +660,1 @@ g_warning("JS ERROR: %s: %s\n%s", utf8_message, utf8_exception, utf8_stack); utf8_message.get() and utf8_stack.get() @@ +660,3 @@ g_warning("JS ERROR: %s: %s\n%s", utf8_message, utf8_exception, utf8_stack); else g_warning("JS ERROR: %s: %s", utf8_message, utf8_exception); utf8_message.get() @@ +665,1 @@ g_warning("JS ERROR: %s\n%s", utf8_exception, utf8_stack); utf8_stack.get() ::: gjs/jsapi-util.h @@ +67,3 @@ + explicit GjsJSFreeArgs(JSContext *cx) : m_cx(cx) + {} + void operator() (char *str) { style nitpick, here and below: blank lines in between methods. @@ +76,3 @@ + std::unique_ptr<char, GjsJSFreeArgs> m_ptr; +public: + JS_free(m_cx, str); cx shouldn't have a default of nullptr. In fact, you could add a g_assert(cx) to the constructor. @@ +86,3 @@ + } + char* release() { +private: At first I thought this would leak memory. I was wrong, but calling it release() is a bit confusing given what std::unique_ptr::release() does. Instead maybe call it copy() and clarify with a comment that the copy must be freed with g_free()? Also possibly implement a different release() function for those cases where we actually want to release the pointer and call JS_free() ourselves? I don't know if there is such a case, so maybe not. @@ +88,3 @@ + return g_strdup(m_ptr.get()); + } + std::unique_ptr<char, GjsJSFreeArgs> m_ptr; I think this overload of reset() should be deleted - we don't want the new string to be associated with the wrong context by accident, we should only pass in a new context and a new string at the same time. @@ +92,3 @@ + } + void reset(JSContext *cx, char *str) { + GjsAutoJSChar(JSContext *cx = nullptr, char *str = nullptr) And in this method you have to do something with cx; maybe give the GjsJSFreeArgs a set_context() method, and call here m_ptr.get_deleter().set_context(cx)? ::: gjs/stack.cpp @@ +102,3 @@ } g_printerr("%s\n", stack); This needs to be stack.get()
Right after publishing the review I figured a way to avoid the copy with the g_bytes_new_take() in coverage.cpp: use g_bytes_new_with_free_func() instead.
Hi Philip, It seems that you are right about the issue in comment #42, and I think it has to do with how pointers are treated on 32-bit systems and 64-bit systems... Sorry, I didn't have the chance to revisit this issue soon, but I will look into this again ASAP, as I am currently away to the United States since roughly about the time you gave the review in comment 43. Thanks though, with blessings!
Created attachment 352347 [details] [review] omplete the transition to std::unique_ptr (take v) Hi Philip, In fact on Ubuntu I had to add the .get() for the code to build and pass... Here's the new patch, I hope I understood you correctly. (In reply to Philip Chimento from comment #43) > Review of attachment 350960 [details] [review] [review]: > Instead of releasing here, I'd change the return type of this function to > GjsAutoJSChar. Since release() actually copies, you could avoid the copy > that way. Unfortunately doing this will cause a C2280 error on Visual Studio, so this won't work for me. > Also possibly implement a different release() function for those cases where > we actually want to release the pointer and call JS_free() ourselves? I > don't know if there is such a case, so maybe not. I don't think we are using this in the code, but added for future's sake. > And in this method you have to do something with cx; maybe give the > GjsJSFreeArgs a set_context() method, and call here > m_ptr.get_deleter().set_context(cx)? I hope I understood you correctly... I left the copy in coverage.cpp for now, anyways... With blessings, thank you!
Review of attachment 352347 [details] [review]: Fantastic. Just a few comments, nothing substantial. If you agree with the comments, then feel free to commit this after fixing them up. I have not had a chance yet to run this under valgrind, but I'm planning to do that anyway later. Thank you for all the hard work. ::: gi/arg.cpp @@ +2549,3 @@ while (g_hash_table_iter_next (&iter, &keyarg.v_pointer, &valarg.v_pointer)) { + nitpick: extraneous blank line ::: gi/object.cpp @@ +864,3 @@ case NO_SUCH_G_PROPERTY: gjs_throw(context, "No property %s on this GObject %s", + name.get(), g_type_name(gtype)); optional nitpick: Since you're touching this line, fix the indentation as well @@ +1609,3 @@ true)) { gjs_throw(context, "No signal '%s' on object '%s'", + signal_name.get(), ditto @@ +1697,3 @@ false)) { gjs_throw(context, "No signal '%s' on object '%s'", + signal_name.get(), ditto @@ +1706,3 @@ if ((argc - 1) != signal_query.n_params) { gjs_throw(context, "Signal '%s' on %s requires %d args got %d", + signal_name.get(), ditto ::: gjs/coverage.cpp @@ +1024,3 @@ + reinterpret_cast<uint8_t*>(statistics_as_json_string.copy()); + + return g_bytes_new_take(json_bytes, I poked at this in order to try to avoid the copy, but it's cumbersome because you have to make a closure struct for g_bytes_new_with_free_func(). I guess this is OK. ::: gjs/jsapi-util-string.cpp @@ +107,3 @@ GError *error; + GjsAutoJSChar tmp(context); + gchar *filename_string; optional nitpick: Since changing this line anyway, change gchar to char. ::: gjs/jsapi-util.h @@ +73,3 @@ + } + + explicit GjsJSFreeArgs(JSContext *cx) : m_cx(cx) I find get() too confusing with GjsAutoJSChar::get(). How about get_context() to correspond with set_context()? @@ +108,3 @@ + /* Strings acquired by this should be JS_free()'ed */ + return JS_strdup(m_ptr.get_deleter().get(), m_ptr.get()); + std::unique_ptr<char, GjsJSFreeArgs> m_ptr; nitpick: Use spaces for indentation here ::: gjs/runtime.cpp @@ +119,3 @@ success = true; out: nitpick: Take this opportunity to remove the out: label and the success variable, and return directly.
Review of attachment 352347 [details] [review]: Hi Philip, Thanks for the feedback along the way! I pushed the patch with your suggestions as bcd1f39. (In reply to Philip Chimento from comment #47) > ::: gjs/coverage.cpp > @@ +1024,3 @@ > + reinterpret_cast<uint8_t*>(statistics_as_json_string.copy()); > + > + return g_bytes_new_take(json_bytes, > > I poked at this in order to try to avoid the copy, but it's cumbersome > because you have to make a closure struct for g_bytes_new_with_free_func(). > I guess this is OK. Yup, that's was why I kept it as-is... With blessings, thank you!
Review of attachment 345723 [details] [review]: Just something I noticed, FWIW ::: gjs/jsapi-util.h @@ +36,3 @@ +class GjsAutoChar : public std::unique_ptr<char, decltype(&g_free)> { +public: + typedef std::unique_ptr<char, decltype(&g_free)> U; The newer "using" keyword is nicer than "typedef", and you should be able to simplify this in any case to using U = unique_ptr as the latter refers to the templated base within this class's scope. Then you might not even feel the need to have a typedef at all, and just write `unique_ptr` in the constructor and operators.
Review of attachment 345723 [details] [review]: ::: gjs/jsapi-util.h @@ +45,3 @@ + + void operator= (const char* str) { + Surely I'm just missing something obvious... but isn't this extremely dangerous? Can't we then pass it a string literal, char const*, which it then loses constness on and will try to free later? That won't end well.
Bah, I clicked on the wrong line. I specifically meant this: > reset(const_cast<char*>(str)); This seems to steal ownership of a string that might be a literal or owned by someone else, so that we'll end up trying to either invalidly or double-free it. I would've expected this `operator=` to create a dynamically allocated copy of the received string and manage the lifetime of the copy.
Created attachment 353553 [details] [review] autofree: Use using declaration instead of typedef C++11 has a nice feature, template aliases, which allow us to make the GjsAutoChar and GjsAutoUnref declarations a bit more readable. Thanks to Daniel Boles for pointing this out.
Created attachment 353554 [details] [review] autofree: Remove GjsAutoChar::operator= This probably should have copied the string, but it was actually not used since the code GjsAutoChar mystring = g_strdup("foo"); will do the right thing due to copy initialization. Since it wasn't needed, just remove it. Thanks to Daniel Boles for pointing this out.
Thanks, Daniel. I've attached two patches with changes. (In reply to Daniel Boles from comment #49) > Then you might not even feel the need to have a typedef at all, and just > write `unique_ptr` in the constructor and operators. Writing `unique_ptr` directly works for GjsAutoChar, but not GjsAutoUnref<T> apparently since it's a template. (In reply to Daniel Boles from comment #51) > I would've expected this `operator=` to create a dynamically allocated copy > of the received string and manage the lifetime of the copy. Hmm, you're probably right. I discovered this operator was never used anyway. However, Chun-wei said it was needed for VS2013 (see the comments about C2280 above). Will this patch still compile with VS2013?
Review of attachment 353553 [details] [review]: Glad my comments were useful! ::: gjs/jsapi-util.h @@ +50,3 @@ class GjsAutoUnref : public std::unique_ptr<T, decltype(&g_object_unref)> { public: + using U = std::unique_ptr<T, decltype(&g_object_unref)>; I think you can avoid this by qualifying that the base type is dependent on the current, derived one - i.e. @@ +54,1 @@ GjsAutoUnref(T *ptr = nullptr) : U(ptr, g_object_unref) {} GjsAutoUnref(T *ptr = nullptr) : GjsAutoUnref::unique_ptr(ptr, g_object_unref) {}
Created attachment 353570 [details] [review] autofree: Avoid using local typedefs in classes We can make the GjsAutoChar and GjsAutoUnref declarations a bit more readable by just writing 'unique_ptr' directly. This is more idiomatic C++ style. Thanks to Daniel Boles for pointing this out.
(In reply to Philip Chimento from comment #54) > (In reply to Daniel Boles from comment #51) > > I would've expected this `operator=` to create a dynamically allocated copy > > of the received string and manage the lifetime of the copy. > > Hmm, you're probably right. I discovered this operator was never used > anyway. However, Chun-wei said it was needed for VS2013 (see the comments > about C2280 above). Will this patch still compile with VS2013? I don't have VS installed so can't test. :( Still, I think the current code that makes it possible to take ownership of a string originally declared const, or which is a const view of something owned by someone else, should not be left in, as it just asks for accidents to happen later, even if it's currently unused. Chun-Wei also proposed just having an operator= for non-const char* as an alternative; I think it's uncertain which one was needed to fix the build in VS. Maybe the char* overload is all that's needed - or possibly also a const char* overload that copies rather than stealing ownership. Still, as you said, I don't know why operator= would be needed if the = syntax is always used in the context of initialisation, as then it should be the constructor that gets called (unless there is some esoteric exception to this rule that I don't know about yet :P)
Hi, Sorry for being late to reply on this issue... Unfortunately the operator()= is still necessary to avoid C2280... The line that was causing this problem is in line 86 of gjs/importer.cpp if we remove the operator()=: output = g_strdup_printf("[%s %s]", klass->name, path.get()); The second patch that makes the code more readable does build on Visual Studio 2013 though. With blessings, thank you!
(In reply to Fan, Chun-wei from comment #58) > Unfortunately the operator()= is still necessary to avoid C2280... > > The line that was causing this problem is in line 86 of gjs/importer.cpp if > we remove the operator()=: > output = g_strdup_printf("[%s %s]", klass->name, path.get()); Oh, so that's not copy-initialisation. You should be able to use operator=(char*), not the operator=(const char*) that is currently in master. That will avoid the potential for accidentally taking ownership of a const char* later. > The second patch that makes the code more readable does build on Visual > Studio 2013 though. This is good news, and almost surprising - but I forget that VC++ is a lot more up to date than VC :-)
Created attachment 353803 [details] [review] GjsAutoChar: Do not take ownership of const char* Does this one build OK?
Hi Daniel, Yes it does :). VC on and after 2013 is now also much better in the C99 department as well--hopefully they do catch up with the C11 standards well. With blessings, and cheers!
Created attachment 353805 [details] [review] GjsAutoChar: Add a proper operator= (const char*) This should work if you want the ability to assign from const char*. It's probably a good idea to have this option, even if it's not needed by any of the current code. - Another thing I noticed is that a number of the conversion operators, get() functions, etc in these classes probably can, and should, be marked as const methods. However, since I can't build Gjs at the moment, I can't say for sure which ones those are. Maybe someone could take a look? Const-correctness is another thing that is probably not needed at the moment but always nice to have!
Created attachment 353807 [details] [review] GjsAutoChar: Add a proper operator= (const char*)
Review of attachment 353554 [details] [review]: Rejecting patch, will not build on VS, as per Fan Chun-wei.
Review of attachment 353803 [details] [review]: +1
Review of attachment 353807 [details] [review]: +1
Thanks, both of you. I'll open a new bug for ensuring const-correctness across all of the C++ objects that we have in GJS. Attachment 353570 [details] pushed as 4d32b0b - autofree: Avoid using local typedefs in classes Attachment 353803 [details] pushed as fb628a9 - GjsAutoChar: Do not take ownership of const char* Attachment 353807 [details] pushed as 2ff21f1 - GjsAutoChar: Add a proper operator= (const char*)