856790 - make nsTArray::EnsureLength() return void if infalible

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: bug 856790 - make nsTArray::EnsureLengthAtleast() return void

Comment 2

[(no longer active)]

Comment on attachment 732033 [details] [diff] [review]
bug 856790 - make nsTArray::EnsureLengthAtleast() return void

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

After you address the comments below, please ask jlebar for the final review since this is his code, and I don't want to end up owning it!

::: xpcom/glue/nsCOMArray.cpp
@@ +173,5 @@
> +  nsISupports *oldObject = mArray[aIndex];
> +  // Make sure to addref first, in case aObject == oldObject
> +  NS_IF_ADDREF(mArray[aIndex] = aObject);
> +  NS_IF_RELEASE(oldObject);
> +  // XXX make this return void

Thanks, and extra points for filing that bug!  ;-)

::: xpcom/glue/nsTArray.h
@@ +158,5 @@
>      return ResultTypeProxy();
>    }
> +
> +  static ResultType UnconditionalSuccess() {
> +    return ResultType();

Drop the return statement, please.

@@ +1219,5 @@
>    // then new elements will be constructed using elem_type's default
>    // constructor.
>    // @param minLen  The desired minimum length of this array.
>    // @return        True if the operation succeeded; false otherwise.
> +typename Alloc::ResultType EnsureLengthAtLeast(size_type minLen) {

Nit: indentation.

@@ +1224,4 @@
>      size_type oldLen = Length();
>      if (minLen > oldLen) {
> +      return Alloc::Result(!!InsertElementsAt(oldLen, minLen - oldLen) ?
> +                           Alloc::SuccessResult() : Alloc::FailureResult());

Hmm, Alloc::Result expects a proxy argument, and InsertElementsAt will _always_ return something (i.e., it will never return void), so using the proxy result infrastructure like this seems like a mistake to me.

Instead, I think you should add a method like |ResultTypeProxy IsValidPointer(void*)| to nsTArrayFallibleAllocatorBase and nsTArrayInfallibleAllocatorBase, and just do this here:

  return Alloc::Result(Alloc::IsValidPointer(InsertElementsAt(...)));

In the infallible implementation of IsValidPointer, you should check the pointer and abort if it's actually null at runtime (similar to nsTArrayInfallibleAllocatorBase::FailureResult.)

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: bug 856790 - make nsTArray::EnsureLengthAtleast() return void

Comment 4

[Justin Lebar (not reading bugmail)]

> this is his code, and I don't want to end up owning it!

Oh, I own tarray now?  :)

Comment 5

[(no longer active)]

(In reply to comment #4)
> > this is his code, and I don't want to end up owning it!
> 
> Oh, I own tarray now?  :)

Well, yeah!  ;)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b47c83333ec7