Closed Bug 1111149 Opened 6 years ago Closed 6 years ago

Move TestArray.cpp to gtest and enable it

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file)

Attached patch Patch (v1)Splinter Review
No description provided.
Attachment #8535957 - Flags: review?(nfroyd)
Comment on attachment 8535957 [details] [diff] [review]
Patch (v1)

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

::: xpcom/glue/tests/gtest/TestArray.cpp
@@ -70,5 @@
> -{
> -  if (aValue1 == aValue2) {
> -    return "OK";
> -  }
> -  if (kExitOnError) {

You should remove the kExitOnError variable, too.

@@ +152,5 @@
>      array->RemoveLastElement(foo);
>      int32_t   removeResult4[9] = {0, 1, 2, 4, 3, 5, 7, 8, 9};
> +    CheckArray(array, 9, removeResult4, 9);
> +
> +    foo = nullptr;

Is this really necessary?  The presence or absence of |foo| shouldn't affect the tests below, so can't we just let it autorelease at the end of the function?
Attachment #8535957 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #1)
> @@ +152,5 @@
> >      array->RemoveLastElement(foo);
> >      int32_t   removeResult4[9] = {0, 1, 2, 4, 3, 5, 7, 8, 9};
> > +    CheckArray(array, 9, removeResult4, 9);
> > +
> > +    foo = nullptr;
> 
> Is this really necessary?  The presence or absence of |foo| shouldn't affect
> the tests below, so can't we just let it autorelease at the end of the
> function?

The number of refs is explicitly tested, so yes this is necessary.  The old version of the test called Release() manually.
Ah, gCount, right.  Carry on!
https://hg.mozilla.org/mozilla-central/rev/7bad83346e58
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.