improve the UniquePtr situation

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments)

UniquePtr is great and I think we should use it everywhere, but it's annoying, verbose and easy to forget the mandatory JS::DeletePolicy/JS::FreePolicy.  We can fix this like js::Vector and have a template alias inside namespace js which changes the default deleter to JS::DeletePolicy.  As with Vector, we avoid ambiguities because, except for one case in xpconnect which this patch fixes, files don't 'using namespace' both 'js' and 'mozilla'.  (Unlike 'JS' which is opened in the same file as 'mozilla' in several places.)

A second patch changes UniqueChars from a UniquePtr<char> to UniquePtr<char[]>, adds a symmetric UniqueTwoByteChars for char16_t[] and applies these two typedefs everywhere.

Together, these two patches remove most of the explicit usage of JS::DeletePolicy/JS::FreePolicy which makes the code look tidier.
Posted patch js-unique-ptrSplinter Review
Note the explicit qualification of mozilla::MakeUnique was added in nsDocShell.cpp because of an ambiguity arising from 'using namespace mozilla' combined with Argument Dependent Lookup of a parameter in namespace JS that was really a typedef for definition in namespace js.
Attachment #8707779 - Flags: review?(jwalden+bmo)
Posted patch unique-charsSplinter Review
Attachment #8707781 - Flags: review?(jdemooij)
Looks green on try.
Comment on attachment 8707781 [details] [diff] [review]
unique-chars

Review of attachment 8707781 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Utility.h
@@ +480,5 @@
>      }
>  };
>  
> +typedef mozilla::UniquePtr<char[], JS::FreePolicy> UniqueChars;
> +typedef mozilla::UniquePtr<char16_t[], JS::FreePolicy> UniqueTwoByteChars;

Nice, so much nicer than having UniquePtr<char16_t[], JS::FreePolicy> copy/pasted everywhere.
Attachment #8707781 - Flags: review?(jdemooij) → review+
Attachment #8707779 - Flags: review?(jwalden+bmo) → review?(jorendorff)
Comment on attachment 8707779 [details] [diff] [review]
js-unique-ptr

Review of attachment 8707779 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. I think this is the right call, and agree UniquePtr and MakeUnique are good stuff.

::: js/public/UniquePtr.h
@@ +54,5 @@
> +typename detail::UniqueSelector<T>::UnknownBound
> +MakeUnique(decltype(sizeof(int)) aN)
> +{
> +  typedef typename mozilla::RemoveExtent<T>::Type ArrayType;
> +  return UniquePtr<T>(js_pod_calloc<ArrayType>(aN));

Constructors aren't called - unlike the mfbt version of this. It seems error prone. I'd be happy if this functionality existed in a separate function, with Pod in the name; or if this signature were just `= delete` (seems likely it's not already being used).

::: js/src/jsapi.cpp
@@ +3985,5 @@
>  static bool
>  Compile(JSContext* cx, const ReadOnlyCompileOptions& options, SyntacticScopeOption scopeOption,
>          const char* bytes, size_t length, MutableHandleScript script)
>  {
> +    UniquePtr<char16_t, JS::FreePolicy> chars;

Oh, yuck. It wouldn't break anything to change this to char16_t[], would it? Just for looks?

@@ +4314,5 @@
>                      const ReadOnlyCompileOptions& options,
>                      const char* name, unsigned nargs, const char* const* argnames,
>                      const char* bytes, size_t length, MutableHandleFunction fun)
>  {
> +    UniquePtr<char16_t, JS::FreePolicy> chars;

Same here.

::: js/src/vm/TraceLogging.h
@@ +121,5 @@
>   * to-report string information.
>   */
>  class TraceLoggerEventPayload {
>      uint32_t textId_;
> +    UniquePtr<char, JS::FreePolicy> string_;

And here. If you're not interested, just let me know it's OK, I can patch 3 lines of code...
Attachment #8707779 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Constructors aren't called - unlike the mfbt version of this. It seems error
> prone. I'd be happy if this functionality existed in a separate function,
> with Pod in the name; or if this signature were just `= delete` (seems
> likely it's not already being used).

Wow, good catch!  I was just mindlessly copypasting.  I'll =delete it for now since we don't even have a delete[] equivalent in JS.

> Oh, yuck. It wouldn't break anything to change this to char16_t[], would it?
> Just for looks?
...
> Same here.
...
> And here. If you're not interested, just let me know it's OK, I can patch 3
> lines of code...

I think you'll especially like the patch jan r+d ;)
https://hg.mozilla.org/mozilla-central/rev/82b49b59162f
https://hg.mozilla.org/mozilla-central/rev/4f02780c73e3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.