Use MOZ_MUST_USE more in SpiderMonkey

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(23 attachments, 2 obsolete attachments)

8.30 KB, patch
jonco
: review+
njn
: checkin+
Details | Diff | Splinter Review
10.36 KB, patch
jonco
: review+
njn
: checkin+
Details | Diff | Splinter Review
72.89 KB, patch
jonco
: review+
njn
: checkin+
Details | Diff | Splinter Review
26.94 KB, patch
bbouvier
: review+
njn
: checkin+
Details | Diff | Splinter Review
4.88 KB, patch
sfink
: review+
njn
: checkin+
Details | Diff | Splinter Review
1.93 KB, patch
jorendorff
: review+
njn
: checkin+
Details | Diff | Splinter Review
77.04 KB, patch
jorendorff
: review+
njn
: checkin+
Details | Diff | Splinter Review
186.63 KB, patch
h4writer
: review+
njn
: checkin+
Details | Diff | Splinter Review
38.54 KB, patch
terrence
: review+
njn
: checkin+
Details | Diff | Splinter Review
11.28 KB, patch
terrence
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.75 KB, patch
h4writer
: review+
njn
: checkin+
Details | Diff | Splinter Review
5.70 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
193.30 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
4.19 KB, patch
jimb
: review+
Details | Diff | Splinter Review
4.57 KB, patch
jimb
: review+
Details | Diff | Splinter Review
4.32 KB, patch
jimb
: review+
Details | Diff | Splinter Review
8.33 KB, patch
jimb
: review+
Details | Diff | Splinter Review
5.60 KB, patch
jimb
: review+
Details | Diff | Splinter Review
5.57 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.23 KB, patch
jimb
: review+
Details | Diff | Splinter Review
2.13 KB, patch
jimb
: review+
Details | Diff | Splinter Review
34.18 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.83 KB, patch
Details | Diff | Splinter Review
I plan to add a *lot* more uses of MOZ_CHECK (added in bug 1267550 as a synonym
for MOZ_WARN_UNUSED_RESULT) to SpiderMonkey. My plan is to be generous:

- Use it for any function returning bool that isn't obviously a predicate (i.e.
  it's answering a yes/no question and has no outparams).

- Don't limit it just to public interfaces, but use it in private interfaces
  where appropriate.
This one found a missing check in jsopcode.cpp.
Attachment #8745198 - Flags: review?(jcoppeard)
Summary: (part 1) - Use MOZ_CHECK more in jsnum.h → Use MOZ_CHECK more in SpiderMonkey
Attachment #8745198 - Flags: review?(jcoppeard) → review+
Summary: Use MOZ_CHECK more in SpiderMonkey → Use MOZ_MUST_USE more in SpiderMonkey
Keywords: leave-open
Attachment #8745198 - Flags: checkin+
Blocks: MOZ_MUST_USE
Duplicate of this bug: 605349
The patch also removes some unnecessary MOZ_MUST_USE annotations on function
definitions (because the function declaration is already annotated).

And it changes the return type of ModuleGenerator::initFuncSig() to void,
because is always returns true.
Attachment #8748987 - Flags: review?(bbouvier)
Attachment #8748989 - Flags: review?(sphink) → review+
This found two missing checks for PackedScopedCoordinate::setSlot(). Fixing one
of them required making ParseContext::updateDecl() fallible.

The patch also changes the return type of addToCallSiteObject() to void.
Attachment #8749021 - Flags: review?(jorendorff)
Attachment #8748965 - Flags: review?(jcoppeard) → review+
Comment on attachment 8748970 [details] [diff] [review]
(part 3) - Use MOZ_MUST_USE more in js/src/builtin/

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

::: js/src/builtin/TypedObject.h
@@ +157,5 @@
>      type::Kind kind() const {
>          return (type::Kind) getReservedSlot(JS_DESCR_SLOT_KIND).toInt32();
>      }
>  
> +    MOZ_MUST_USE bool opaque() const {

This one, transparent() and hasTraceList() below are predicates - comment 1 says you were not intending to annotate these.

@@ +560,5 @@
>          MOZ_ASSERT(offset <= (size_t) size());
>          return typedMem() + offset;
>      }
>  
> +    inline MOZ_MUST_USE bool opaque() const;

Ditto here.
Attachment #8748970 - Flags: review?(jcoppeard) → review+
Attachment #8749005 - Flags: review?(jorendorff) → review+
Comment on attachment 8749021 [details] [diff] [review]
(part 7) - Use MOZ_MUST_USE more in js/src/ctypes/

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

::: js/src/frontend/BytecodeEmitter.h
@@ +37,5 @@
>      Vector<Value> list;
>    public:
>      explicit CGConstList(ExclusiveContext* cx) : list(cx) {}
> +    MOZ_MUST_USE bool append(Value v) {
> +        MOZ_ASSERT_IF(v.isString(), v.toString()->isAtom()); return list.append(v);

Style nit: Line break after the first `;`, please.
Attachment #8749021 - Flags: review?(jorendorff) → review+
terrence, there are numerous GC functions that appear to be falliable but
aren't checked at all callsites, and I can't tell if they are dangerous or not.
Can you please take a look? They're all marked with "njn: ?" comments and a
|(void)| cast, which is enough to stop clang from complaining, though not
enough to stop GCC complaining.
Attachment #8750166 - Flags: feedback?(terrence)
Comment on attachment 8748987 [details] [diff] [review]
(part 4) - Use MOZ_MUST_USE more in js/src/asmjs/

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

Thank you for doing this.
Attachment #8748987 - Flags: review?(bbouvier) → review+
Attachment #8748965 - Flags: checkin+
Attachment #8748970 - Flags: checkin+
Attachment #8748989 - Flags: checkin+
Attachment #8749005 - Flags: checkin+
Attachment #8749021 - Flags: checkin+
Attachment #8748987 - Flags: checkin+
Comment on attachment 8750624 [details] [diff] [review]
(part 9) - Use MOZ_MUST_USE more in js/src/jit/

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

Nice work! And you found already some places that weren't testing the return value.
Thanks for fixing the arguments list length within the rules throughout the patch.

::: js/src/jit/IonAnalysis.cpp
@@ +3523,5 @@
>      for (size_t i = 0; i < lhs.numTerms(); i++) {
>          if (lhs.term(i).scale == -1) {
>              rhsDef = lhs.term(i).term;
> +            // njn: ?
> +            (void)lhs.add(rhsDef, 1);

Can you add above
AutoEnterOOMUnsafeRegion oomUnsafe;

and crash here?
if (!lhs.add(rhsDef, 1))
   oomUnsafe.crash("ConvertLinearInequality");

I agree it is not the best solution, but it is better than ignoring it. We should probably try to remove all these crash places. Note: loopunroller is not enabled. So we shouldn't see any crashes here.

::: js/src/jit/IonBuilder.h
@@ +341,5 @@
>      void rewriteParameter(uint32_t slotIdx, MDefinition* param, int32_t argIndex);
>      void rewriteParameters();
> +    MOZ_MUST_USE bool initScopeChain(MDefinition* callee = nullptr);
> +    MOZ_MUST_USE bool initArgumentsObject();
> +    bool pushConstant(const Value& v);  // always returns true

Can we also add a "MOZ_MUST_USE" here? We added it to be consistent, making it "easier" not to forget checking the return value.
Attachment #8750624 - Flags: review?(hv1989) → review+
> > +    bool pushConstant(const Value& v);  // always returns true
> 
> Can we also add a "MOZ_MUST_USE" here? We added it to be consistent, making
> it "easier" not to forget checking the return value.

I did that, but there are quite a few (~10 or so) places where the return value isn't checked. Which is fair enough, given that it always returns true, and I didn't want to add a bunch of redundant checks.

I'm not much of a fan of the "always return the same value just so you can use the function in a return statement" approach. It's one of those "clever" approaches that makes code more concise but harder to understand -- you have to look at the definition of pushConstant() to see that it always returns true. I could just change the return type to void...
(In reply to Nicholas Nethercote [:njn] from comment #18)
> > > +    bool pushConstant(const Value& v);  // always returns true
> > 
> > Can we also add a "MOZ_MUST_USE" here? We added it to be consistent, making
> > it "easier" not to forget checking the return value.
> 
> I did that, but there are quite a few (~10 or so) places where the return
> value isn't checked. Which is fair enough, given that it always returns
> true, and I didn't want to add a bunch of redundant checks.
> 
> I'm not much of a fan of the "always return the same value just so you can
> use the function in a return statement" approach. It's one of those "clever"
> approaches that makes code more concise but harder to understand -- you have
> to look at the definition of pushConstant() to see that it always returns
> true. I could just change the return type to void...

Just to be clear. The "clever" part was not "to use the function in a return statement", but similar function calls all needed to be "return checked". And this "pushConstant" was misfit. Having it also return checked made sure that it was the default approach to return check all those function (in theory). Like you saw that didn't always happen.

I'm fine to make it void, since we now have asserts that will make sure the functions are return checked.
Comment on attachment 8750166 [details] [diff] [review]
(part 8) - Use MOZ_MUST_USE more in js/src/gc/

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

Wow, this is great: it sheds light on the really, really ancient and ugly parts of the GC.

::: js/src/gc/Allocator.cpp
@@ +31,5 @@
>      // Invoking the interrupt callback can fail and we can't usefully
>      // handle that here. Just check in case we need to collect instead.
>      if (rt->hasPendingInterrupt())
> +        // njn: ?
> +        (void)gcIfRequested(cx);

The return of gcIfRequested is whether or not we did a major GC. (void) is correct here as we don't care if we GC'd in this particular case, but we do in other cases where we can skip additional need-to-GC checks.

::: js/src/gc/GCRuntime.h
@@ +626,5 @@
>      void triggerFullGCForAtoms() {
>          MOZ_ASSERT(fullGCForAtomsRequested_);
>          fullGCForAtomsRequested_ = false;
> +        // njn: ?
> +        (void)triggerGC(JS::gcreason::ALLOC_TRIGGER);

I believe this should be MOZ_RELEASE_ASSERT(triggerGC(...)). I have no idea what this interface is even for. I'll file a new bug to figure it out.

::: js/src/gc/Heap.h
@@ +1267,5 @@
>          MOZ_ASSERT(tmp == thing);
>      }
>      if (thing->isMarked(GRAY))
> +        // njn: ?
> +        (void)UnmarkGrayCellRecursively(thing, thing->getTraceKind());

UnmarkGray et.al. return whether anything was actually unmarked. The Cycle Collector uses this to UnmarkGray WeakMaps to a fixed point. In this case we don't care: if the reference is stored in a weakmap, the later pass will handle it. I think (void) is the right approach here.

::: js/src/gc/Marking.cpp
@@ +578,5 @@
>      // permanent atoms, so likewise require no subsquent marking.
>      CheckTracedThing(trc, *ConvertToBase(&thing));
>      if (trc->isMarkingTracer())
> +        // njn: ?
> +        (void)thing->markIfUnmarked(gc::BLACK);

|markIfUnmarked| returns true if the thing went from unmarked to marked: e.g. if we need to mark subsequent edges. We know we do not in this case because atoms and symbols do not have children, as the comment immediately above explains. I think (void) on is the right choice here.

@@ +1909,5 @@
>          for (ArenaCellIterUnderGC i(arena); !i.done(); i.next()) {
>              TenuredCell* t = i.getCell();
>              if (always || t->isMarked()) {
> +                // njn: ?
> +                (void)t->markIfUnmarked();

In this case we don't care about the prior mark status because we're do TraceChildren immediately afterwards unconditionally. (void) is fine here too.

::: js/src/gc/Nursery.cpp
@@ +393,5 @@
>           * safe to keep these entries as they may refer to tenured cells which
>           * may be freed after this point.
>           */
> +        // njn: ?
> +        (void)sb.clear();

Clear used to be fallible. It looks like I did not fix the wrapper call when I moved the child buffers to infallible interfaces. The proper solution is to make StoreBuffer::clear return void.

@@ +492,5 @@
>      TIME_END(sweep);
>  
>      TIME_START(clearStoreBuffer);
> +    // njn: ?
> +    (void)rt->gc.storeBuffer.clear();

As above.

::: js/src/gc/StoreBuffer.h
@@ +409,5 @@
>      bool enable();
>      void disable();
>      bool isEnabled() const { return enabled_; }
>  
> +    MOZ_MUST_USE bool clear();

Make this return void. The implementation will be trivial to fix.

::: js/src/gc/Verifier.cpp
@@ +366,5 @@
>  gc::GCRuntime::verifyPreBarriers()
>  {
>      if (verifyPreData)
> +        // njn: ?
> +        (void)endVerifyPreBarriers();

It seems like the return is the same as |bool(verifyPreData)|, so I think we should make this return void, as for endVerifyPostBarriers.

::: js/src/jsgc.cpp
@@ +804,5 @@
>  void Chunk::decommitAllArenas(JSRuntime* rt)
>  {
>      decommittedArenas.clear(true);
> +    // njn: ?
> +    (void)MarkPagesUnused(&arenas[0], ArenasPerChunk * ArenaSize);

The madvise we do here is, as per the name, advisory. If we fail to decommit, it's not a problem from our side, the system just has less available memory. We used to MOZ_RELEASE_ASSERT here, but it turns out that linux can fail the madvise syscall if there is not enough memory available. I think a (void) annotation is probably the right way to go.

@@ +868,5 @@
>      decommittedArenas.unset(offset);
>  
>      Arena* arena = &arenas[offset];
> +    // njn: ?
> +    (void)MarkPagesInUse(arena, ArenaSize);

This is also advisory. The system will fault our pages in (or oom kill the process) as soon as we touch the pages regardless of the status of the call. At least in this case the platform could feasibly decide to use less memory. We don't really have that option though, so we want to just crash when we touch the page next. I think (void) is the right annotation here, although MOZ_RELEASE_ASSERT might also work. I think I'd prefer the (void) annotation -- systems with <4KiB page size could live a little longer that way.

@@ +3305,5 @@
>      if (usedBytes >= thresholdBytes) {
>          // The threshold has been surpassed, immediately trigger a GC,
>          // which will be done non-incrementally.
> +        // njn: ?
> +        (void)triggerZoneGC(zone, JS::gcreason::ALLOC_TRIGGER);

The return of triggerZoneGC tells us whether we actually were able to do the GC. These allocation triggers are advisory and might commonly fail, so I think (void) is the right approach here.

@@ +3319,5 @@
>              // to try to avoid performing non-incremental GCs on zones
>              // which allocate a lot of data, even when incremental slices
>              // can't be triggered via scheduling in the event loop.
> +            // njn: ?
> +            (void)triggerZoneGC(zone, JS::gcreason::ALLOC_TRIGGER);

Same here.

@@ +3357,5 @@
>              fullGCForAtomsRequested_ = true;
>              return false;
>          }
> +        // njn: ?
> +        (void)triggerGC(reason);

And this where we might set the weird fullGCForAtomsRequested flag. We don't care about the return here either.

@@ +4286,5 @@
>  
>      // TODO bug 1167452: Make weak marking incremental
>      SliceBudget budget = SliceBudget::unlimited();
> +    // njn: ?
> +    (void)marker.drainMarkStack(budget);

We can only return false if we are over-budget, which should be impossible with an unlimited budget. This should be MOZ_RELEASE_ASSERT and |budget| should be called |unlimited|.

@@ +4306,5 @@
>              break;
>  
>          auto unlimited = SliceBudget::unlimited();
> +        // njn: ?
> +        (void)marker.drainMarkStack(unlimited);

MOZ_RELEASE_ASSERT.

@@ +4334,5 @@
>              (*op)(&marker, grayRootTracer.data);
>      }
>      auto unlimited = SliceBudget::unlimited();
> +    // njn: ?
> +    (void)marker.drainMarkStack(unlimited);

MOZ_RELEASE_ASSERT.

@@ +4498,5 @@
>  
>          auto unlimited = SliceBudget::unlimited();
>          gc->incrementalState = MARK;
> +        // njn: ?
> +        (void)gc->marker.drainMarkStack(unlimited);

MOZ_RELEASE_ASSERT.

@@ +4960,5 @@
>      }
>  
>      auto unlimited = SliceBudget::unlimited();
> +    // njn: ?
> +    (void)rt->gc.marker.drainMarkStack(unlimited);

MOZ_RELEASE_ASSERT.

::: js/src/jspropertytree.cpp
@@ +174,5 @@
>              parent->removeChild(existingShape);
>              existingShape = nullptr;
>          } else if (existingShape->isMarked(gc::GRAY)) {
> +            // njn: ?
> +            (void)UnmarkGrayShapeRecursively(existingShape);

As for the other other instance, a (void) annotation is fine.

::: js/src/vm/Interpreter.cpp
@@ +433,5 @@
>  {
>      JSRuntime* runtime;
>      explicit AutoGCIfRequested(JSRuntime* rt) : runtime(rt) {}
> +    // njn: ?
> +    ~AutoGCIfRequested() { (void)runtime->gc.gcIfRequested(); }

As with the other instance, a (void) annotation is fine.

::: js/src/vm/Runtime.cpp
@@ +571,5 @@
>  {
>      MOZ_ASSERT(cx->runtime()->requestDepth >= 1);
>  
> +    // njn: ?
> +    (void)cx->runtime()->gc.gcIfRequested();

The following checks do not care if we GC here, so a (void) annotation is fine.
Attachment #8750166 - Flags: feedback?(terrence) → feedback+
Thank you for the detailed response. I've addressed all your comments.

For the following functions, I removed the MOZ_MUST_USE annotation and added
comments to the following to make clear that the bool return value indicates
something other than success/failure:
- GCRuntime::gcIfRequested()
- GCRuntime::triggerZoneGC()
- UnmarkGray{Cell,GCThing,Shape,Recursively}()
- ChunkBitmap::markIfUnmarked()
- TenuredCell::markIfUnmarked()

For this one I just removed the MOZ_MUST_USE annotation, because not checking
for failure is reasonable:
- MarkPagesUnused()

I changed these ones to return void:
- StoreBuffer::clear()
- endVerifyPreBarriers, which required tweaking AutoStopVerifyingBarriers'
  constructor
- MarkPagesInUse: all instances always returned true, and the one call site
  didn't check the return value.

I used MOZ_RELEASE_ASSERT on these ones:
- drainMarkStack() (with unlimited budget)
- triggerGC()
Attachment #8752002 - Flags: review?(terrence)
Attachment #8750166 - Attachment is obsolete: true
Comment on attachment 8752002 [details] [diff] [review]
(part 8) - Use MOZ_MUST_USE more in js/src/gc/

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

Thanks for doing this work! It's going to be a pain to rebase around, but well worth it.

::: js/src/gc/GCRuntime.h
@@ +1054,5 @@
>       */
>      mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> numArenasFreeCommitted;
>      VerifyPreTracer* verifyPreData;
> +  public:
> +    bool hasVerifyPreData() const { return !!verifyPreData; }

This should be added just under startVerifyPreBarrier() in the public section above. Actually, it looks like it already exists with a longer name: isVerifyPreBarriersEnabled Please just use that.

::: js/src/gc/StoreBuffer.cpp
@@ +80,5 @@
>      bufferSlot.clear();
>      bufferWholeCell.clear();
>      bufferGeneric.clear();
>  
> +    return;

Please just elide the trailing return.
Attachment #8752002 - Flags: review?(terrence) → review+
Attachment #8750624 - Flags: checkin+
Attachment #8752002 - Flags: checkin+
This patch fixes numerous unchecked calls.
Attachment #8758557 - Flags: review?(terrence)
Comment on attachment 8758557 [details] [diff] [review]
(part 10) - Use MOZ_MUST_USE in AutoVectorRooteBase

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

Good find!
Attachment #8758557 - Flags: review?(terrence) → review+
Attachment #8758557 - Flags: checkin+
This catches a missing check.
Attachment #8759529 - Flags: review?(hv1989)
Attachment #8759529 - Flags: review?(hv1989) → review+
Attachment #8759529 - Flags: checkin+
This patch contains all the new MOZ_MUST_USE annotations, and I just used
|(void)| to fix the places that currently miss checks. The next patch will fix
those up, and I will merge the patches before landing. They're currently
separate to help make reviewing easier.
Attachment #8761426 - Flags: review?(hv1989)
Sorry, I uploaded the two patches in the wrong order. *This* one adds the
annotations, the other one fixes things up.
Attachment #8761428 - Flags: review?(hv1989)
Comment on attachment 8761426 [details] [diff] [review]
(part 12b) - Use MOZ_MUST_USE even more in js/src/jit/

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

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +530,5 @@
>          alloc.setNeedSideEffect();
>  
> +    AutoEnterOOMUnsafeRegion oomUnsafe;
> +    if (!snapshots_.add(alloc))
> +       oomUnsafe.crash("CodeGeneratorShared::encodeAllocation");

No need to crash here.
Please use: masm.propagateOOM();
Attachment #8761426 - Flags: review?(hv1989) → review+
Comment on attachment 8761426 [details] [diff] [review]
(part 12b) - Use MOZ_MUST_USE even more in js/src/jit/

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

::: js/src/jit/RegisterAllocator.cpp
@@ +172,4 @@
>  
>                  while (!worklist.empty()) {
>                      IntegrityItem item = worklist.popCopy();
> +                    if (!checkIntegrity(item.block, *item.block->rbegin(), item.vreg, item.alloc, populateSafepoints))

Pre-existing nit:
can you cut this line into two to fit the 80 column width?

if (!checkIntegrity(item.block, *item.block->rbegin(), item.vreg, item.alloc,
                    populateSafepoints))
{
    return false;
}
Attachment #8761428 - Flags: review?(hv1989) → review+
> can you cut this line into two to fit the 80 column width?

I will cut it to fit the 100 column width :)
It just makes unrelated changes mess up blame when they fix indentation, or it
makes the indentation go bad to preserve blame. Neither is a good outcome...
Attachment #8762391 - Flags: review?(jimb)
njn: I hope I am not stepping on your toes! I just figured that I could help speed up the transition with the parts of js I am familiar with (and I'm not going to get anything that requires deep thought done while waiting for my plane to board...)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #54)
> njn: I hope I am not stepping on your toes! I just figured that I could help
> speed up the transition with the parts of js I am familiar with (and I'm not
> going to get anything that requires deep thought done while waiting for my
> plane to board...)

It's a big enough job that I'm happy to share it :)  Though it is a little odd having two people contributing patches in the same bug. This one probably should be closed -- I've dragged it out long enough -- and start a new one when I next work on this again. Would you mind closing it once you land your patches?
Attachment #8762389 - Flags: review?(jimb) → review+
Attachment #8762390 - Flags: review?(jimb) → review+
Attachment #8762391 - Flags: review?(jimb) → review+
Attachment #8762392 - Flags: review?(jimb) → review+
Attachment #8762393 - Flags: review?(jimb) → review+
Attachment #8762394 - Flags: review?(jimb) → review+
Attachment #8762395 - Flags: review?(jimb) → review+
Attachment #8762396 - Flags: review?(jimb) → review+
Attachment #8762397 - Flags: review?(jimb) → review+
Comment on attachment 8762398 [details] [diff] [review]
Use MOZ_MUST_USE in js/public/Debug.h

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

::: js/public/Debug.h
@@ +281,5 @@
>  
>  
>  // For each Debugger that observed a debuggee involved in the given GC event,
>  // call its `onGarbageCollection` hook.
> +JS_PUBLIC_API(MOZ_MUST_USE bool)

You told me this works on all the try targets, but I think it would be better for MOZ_MUST_USE to appear outside the JS_PUBLIC_API call. This just looks really weird.
Attachment #8762398 - Flags: review?(jimb) → review-
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/991141e6f910
Use MOZ_MUST_USE in js/src/vm/SavedFrame.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb71788bdd54
Use MOZ_MUST_USE in js/src/vm/SavedStacks.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e75159d1c1e9
Stop trying to align method declarations in js/src/vm/SavedStacks.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cf94fa7665
Use MOZ_MUST_USE in js/public/UbiNode.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4de4115e38
Use MOZ_MUST_USE in js/public/UbiNodeCensus.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6296c3d3ad
Use MOZ_MUST_USE in js/public/UbiNodeDominatorTree.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2129a04a13
Use MOZ_MUST_USE in js/public/UbiNodePostOrder.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ddf8805e824
Use MOZ_MUST_USE in js/public/UbiNodeShortestPaths.h; r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/53826630a9c1
Use MOZ_MUST_USE in js/src/vm/Debugger.h; r=jimb
Ok, fixed the ordering with JS_PUBLIC_API.
Attachment #8763225 - Flags: review?(jimb)
Attachment #8762398 - Attachment is obsolete: true
fitzgen: BTW, we should put this work on hold while jorendorff is working on bug 1277368, because mozilla::Result will hopefully replace all these bool return values, and mozilla::Result has MOZ_MUST_USE_TYPE on it.
SGTM, can't wait for the errormageddon :)
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.