Closed Bug 856790 Opened 7 years ago Closed 7 years ago

make nsTArray::EnsureLength() return void if infalible

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(2 files)

No description provided.
Depends on: 856794
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.)
Attachment #732033 - Flags: review?(ehsan)
> this is his code, and I don't want to end up owning it!

Oh, I own tarray now?  :)
Attachment #734834 - Flags: review?(justin.lebar+bug) → review+
(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!  ;)
Depends on: 859646
https://hg.mozilla.org/mozilla-central/rev/b47c83333ec7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.