GNOME Bugzilla – Bug 785424
Patch: backport of changes required for use with mozjs-55
Last modified: 2017-07-31 14:22:47 UTC
Created attachment 356392 [details] [review] Backport of patch required for mozjs-55 git log -------------------------------------------------- mozjs52+: Backport of changes required for mozjs55 In mozjs-55 there are a few changes internally that require changes to the way things are handled by GJS: vector resizing operations that return a value now have MOZ_MUST_USE, and Rooted<T> + Handle<T> have changes which require comparing to nullptr instead of NULL. The changes to GJS for the above are safe for use in the mozjs-52 version of GJS and have been backported. --------------------------------------------------
Review of attachment 356392 [details] [review]: Hi Luke, I already looked at this patch in general when you showed it to me the other day, so +1. But would you mind splitting it up into two patches as mentioned on IRC? One for the nullptr changes, and one for the MOZ_MUST_USE changes. (Use `git reset HEAD^` to "uncommit" it while keeping the changes, and `git add -p` on each file to selectively add changes.)
Created attachment 356495 [details] [review] Backport of patch required for JSObject compare to nullptr
Created attachment 356496 [details] [review] Backport of patch required for vectors MOZ_MUST_USE returns
Hi Philip, Very sorry, I had forgotten about that (and learned something new about git). Hopefully the patches are now what you were asking for.
Review of attachment 356495 [details] [review]: I realized a couple of things while reviewing that would make this patch better. 1) Instead of comparing against nullptr explicitly, let's use `if (thing)` and `if (!thing)` so that we can just take advantage of convert-to-bool operators. I have been doing this already in most of the code that I've been writing and it's probably good to make it an official style preference. 2) I realized that the cause for why you had to add the `.get()` was probably that GjsMaybeOwned<T>'s operator== takes a T* pointer. I think if you add an overload for JS::Handle<T> then the errors will just go away and you won't have to add `.get()`. i.e. inline bool operator==(JS::Handle<T> other) { return *this == other.get(); } If both of these suggestions still work on your mozjs55 branch, then I think that's what I'd prefer to merge. ::: gjs/jsapi-class.h @@ +308,3 @@ } \ object = JS_NewObjectForConstructor(context, &gjs_##name##_class, argv); \ + if (object == nullptr) \ Make sure the \'s still line up as much as possible if you are modifying the line. ::: gjs/jsapi-util.cpp @@ +316,3 @@ gjs_build_string_array(context, array_length, (char **) array_values)); + if (array == nullptr) Most of the changes in this file aren't needed for the JS::Rooted / JS::Handle change.
Review of attachment 356496 [details] [review]: Just a couple of comments ::: gi/arg.cpp @@ +592,3 @@ + if (!elems.growBy(1)) { + g_error("out of memory in gjs_array_from_strv"); + return false; No need for `return false` after `g_error()`: it aborts the program. (You can remove the curly braces from all these blocks if there's only one line in the body of the if statement.) @@ +2471,3 @@ arg.v_##type = array[i]; \ + if (!elems.growBy(1)) { \ + gjs_##throw(context, "out of memory in gjs_array_from_g_list"); \ g_error
::: gi/arg.cpp @@ +592,3 @@ + if (!elems.growBy(1)) { + g_error("out of memory in gjs_array_from_strv"); + return false; I must have missed that one when I converted from gjs_throw. I'll get cracking on your other suggestions too
With inline bool operator!=(const T& other) and inline bool operator!=(JS::Handle<T> other) (plus the ==) I get ambiguous operators, so I've left that out. The two patches have been run through the cheese grater and cleaned up a bit with your suggestions.
Created attachment 356585 [details] [review] Backport of patch required for JSObject compare to nullptr
Created attachment 356586 [details] [review] Backport of patch required for vectors MOZ_MUST_USE returns
Review of attachment 356585 [details] [review]: A couple of stray whitespace changes, but I will fix it and push anyway. Thanks.
Review of attachment 356586 [details] [review]: I think you might have missed the comment about the curly braces. All the one-line bodies of the if statements don't need braces. In addition there's one thing I forgot to comment on last time: the error messages will already include the information about the file and line in the system log, so no need to repeat the function name and expression in each error message. "Out of memory" or "Unable to append to vector" is probably enough. (And the location information will become stale if code is moved around.) ::: gi/value.cpp @@ +223,2 @@ JS::AutoValueVector argv(context); + if (!argv.reserve(n_param_values)) { Let's keep the comment here, I think it's still informative. (It refers to sometimes needing less space than is reserved here.) ::: gjs/importer.cpp @@ +397,3 @@ + if (!prop_ids.append(ids[ix])) { + g_error("prop_ids.append(), out of memory in load_module_elements"); + return; This return isn't needed. ::: gjs/jsapi-util.cpp @@ +846,3 @@ + if (!scope_chain.append(eval_obj)) { + g_error("gjs_eval_with_scope(), out of memory, unable to append to vector "); + return false; This return isn't needed.
Comment on attachment 356585 [details] [review] Backport of patch required for JSObject compare to nullptr Attachment 356585 [details] pushed as c1bc509 - Backport of patch required for JSObject compare to nullptr
Created attachment 356614 [details] [review] Backport of patch required for vectors MOZ_MUST_USE returns Much cleaned up, properly caught many issues, used suggestions.
Review of attachment 356614 [details] [review]: Looks great. I had a few nitpicks but I'll just fix them as I push it. Thanks! ::: gi/arg.cpp @@ +2463,3 @@ arg.v_##type = array[i]; \ + if (!elems.growBy(1)) \ + g_##error("Unable to grow vector"); \ No need for ## here - it's not an escape for _, it's an obscure corner of the preprocessor. To find out more, read https://en.wikipedia.org/wiki/C_preprocessor#Token_concatenation ::: gjs/jsapi-util.cpp @@ +843,3 @@ JS::AutoObjectVector scope_chain(context); + if (!scope_chain.append(eval_obj)) + g_error("Unable to append to vector "); Delete extra space
Attachment 356614 [details] pushed as e6cc09d - Backport of patch required for vectors MOZ_MUST_USE returns