Nicholas Nethercote [inactive] Assignee
	Description • 8 years ago

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.

	Nicholas Nethercote [inactive] Assignee
	Comment 1 • 8 years ago

This one found a missing check in jsopcode.cpp.

	Nicholas Nethercote [inactive] Assignee
	Comment 2 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e791431d1864b4609b43533fe7d4351e192a9e46
Bug 1267551 (part 1) - Use MOZ_MUST_USE more in jsnum.h. r=jonco.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 4 • 8 years ago
bugherder

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

	Nicholas Nethercote [inactive] Assignee
	Comment 7 • 8 years ago

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.

	Nicholas Nethercote [inactive] Assignee
	Comment 10 • 8 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 11 • 8 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 12 • 8 years ago

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.

	Nicholas Nethercote [inactive] Assignee
	Comment 13 • 8 years ago

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.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 14 • 8 years ago

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.

	Nicholas Nethercote [inactive] Assignee
	Comment 15 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6254d5c31524b7acbad7662c2ec656c3315952
Bug 1267551 (part 2) - Use MOZ_MUST_USE more in js/src/ds/. r=jonco.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0eeeb82335526f5a2c6226c5317b9b691f4ed9c
Bug 1267551 (part 3) - Use MOZ_MUST_USE more in js/src/builtin/. r=jonco.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b115b854171f01c1a521e389f2c52c700e9f935
Bug 1267551 (part 4) - Use MOZ_MUST_USE more in js/src/asmjs/. r=bbouvier.

https://hg.mozilla.org/integration/mozilla-inbound/rev/75ceb95a4cb101b3b706780ecbdc825696891dcd
Bug 1267551 (part 5) - Use MOZ_MUST_USE more in js/src/ctypes/. r=sfink.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6a8e6f170fbd1028f0231f7c651644915159a7
Bug 1267551 (part 6) - Remove dead GenerateBlockId declaration. r=jorendorff.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e457582d0089f8efa6b89f112298373aa50e6e5f
Bug 1267551 (part 7) - Use MOZ_MUST_USE more in js/src/frontend/. r=jorendorff.

	Hannes Verschore [:h4writer]
	Comment 17 • 8 years ago

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.

	Nicholas Nethercote [inactive] Assignee
	Comment 18 • 8 years ago

> > +    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...

	Hannes Verschore [:h4writer]
	Comment 19 • 8 years ago

(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.

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9b6254d5c315
https://hg.mozilla.org/mozilla-central/rev/a0eeeb823355
https://hg.mozilla.org/mozilla-central/rev/5b115b854171
https://hg.mozilla.org/mozilla-central/rev/75ceb95a4cb1
https://hg.mozilla.org/mozilla-central/rev/bd6a8e6f170f
https://hg.mozilla.org/mozilla-central/rev/e457582d0089

	Terrence Cole [:terrence]
	Comment 21 • 8 years ago

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.

	Nicholas Nethercote [inactive] Assignee
	Comment 22 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/47dcb8959eed895257ede693cc7b10d8ebceaf50
Bug 1267551 (part 9) - Use MOZ_MUST_USE more in js/src/jit/. r=h4writer.

	Carsten Book [:Tomcat]
	Comment 23 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/47dcb8959eed

	Nicholas Nethercote [inactive] Assignee
	Comment 24 • 8 years ago

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()

	Terrence Cole [:terrence]
	Comment 25 • 8 years ago

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.

	Nicholas Nethercote [inactive] Assignee
	Comment 26 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/742dca602ca36ee05959b271db99a0f676bba436
Bug 1267551 (part 8) - Use MOZ_MUST_USE more in js/src/gc/. r=terrence.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 27 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/742dca602ca3

	Nicholas Nethercote [inactive] Assignee
	Comment 28 • 8 years ago

This patch fixes numerous unchecked calls.

	Terrence Cole [:terrence]
	Comment 29 • 8 years ago

Comment on attachment 8758557 [details] [diff] [review]
(part 10) - Use MOZ_MUST_USE in AutoVectorRooteBase

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

Good find!

	Nicholas Nethercote [inactive] Assignee
	Comment 30 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3c2afcc1d78153c4ace5b41e7c238c4d54ee8d
Bug 1267551 (part 10) - Use MOZ_MUST_USE in AutoVectorRooterBase. r=terrence.

	Nicholas Nethercote [inactive] Assignee
	Comment 31 • 8 years ago

This catches a missing check.

	Carsten Book [:Tomcat]
	Comment 32 • 8 years ago
bugherder

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

	Nicholas Nethercote [inactive] Assignee
	Comment 33 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc00bd9f0d0919c7b350862b9b44239a7d5bd837
Bug 1267551 (part 11) - Use MOZ_MUST_USE in js/src/vm/Printer.h. r=h4writer.

	Nicholas Nethercote [inactive] Assignee
	Comment 34 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a87ed5532b9e49a8ada850d80b39ca12f512c5dc
Bug 1267551 (part 11b) - Follow-up to fix Android bustage. r=me.

	Carsten Book [:Tomcat]
	Comment 35 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fc00bd9f0d09
https://hg.mozilla.org/mozilla-central/rev/a87ed5532b9e

	Nicholas Nethercote [inactive] Assignee
	Comment 36 • 8 years ago

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.

	Nicholas Nethercote [inactive] Assignee
	Comment 37 • 8 years ago

Sorry, I uploaded the two patches in the wrong order. *This* one adds the
annotations, the other one fixes things up.

	Hannes Verschore [:h4writer]
	Comment 38 • 8 years ago

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();

	Hannes Verschore [:h4writer]
	Comment 39 • 8 years ago

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;
}

	Nicholas Nethercote [inactive] Assignee
	Comment 40 • 8 years ago

> can you cut this line into two to fit the 80 column width?

I will cut it to fit the 100 column width :)

	Nicholas Nethercote [inactive] Assignee
	Comment 41 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8dac921f07452ec836233b7ccc1f0b194b3d854
Bug 1267551 (part 12) - Use MOZ_MUST_USE even more in js/src/jit/. r=h4writer.

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago
bugherder

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 45 • 8 years ago

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...

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 53 • 8 years ago

Try push for my queue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d8a2e50a77

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 54 • 8 years ago

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...)

	Nicholas Nethercote [inactive] Assignee
	Comment 55 • 8 years ago

(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?

	Jim Blandy :jimb
	Comment 56 • 8 years ago

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.

	Pulsebot
	Comment 57 • 8 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 58 • 8 years ago

Ok, fixed the ordering with JS_PUBLIC_API.

	Nicholas Nethercote [inactive] Assignee
	Comment 59 • 8 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 60 • 8 years ago

SGTM, can't wait for the errormageddon :)

	Carsten Book [:Tomcat]
	Comment 61 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/991141e6f910
https://hg.mozilla.org/mozilla-central/rev/eb71788bdd54
https://hg.mozilla.org/mozilla-central/rev/e75159d1c1e9
https://hg.mozilla.org/mozilla-central/rev/f6cf94fa7665
https://hg.mozilla.org/mozilla-central/rev/7c4de4115e38
https://hg.mozilla.org/mozilla-central/rev/6b6296c3d3ad
https://hg.mozilla.org/mozilla-central/rev/ee2129a04a13
https://hg.mozilla.org/mozilla-central/rev/7ddf8805e824
https://hg.mozilla.org/mozilla-central/rev/53826630a9c1