Closed
Bug 1267551
Opened 8 years ago
Closed 7 years ago
Use MOZ_MUST_USE more in SpiderMonkey
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
Attachments
(23 files, 2 obsolete files)
8.30 KB,
patch
|
jonco
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
10.36 KB,
patch
|
jonco
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
72.89 KB,
patch
|
jonco
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
26.94 KB,
patch
|
bbouvier
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
4.88 KB,
patch
|
sfink
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
jorendorff
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
77.04 KB,
patch
|
jorendorff
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
186.63 KB,
patch
|
h4writer
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
38.54 KB,
patch
|
terrence
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
11.28 KB,
patch
|
terrence
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
2.75 KB,
patch
|
h4writer
:
review+
n.nethercote
:
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.
![]() |
Assignee | |
Comment 1•8 years ago
|
||
This one found a missing check in jsopcode.cpp.
Attachment #8745198 -
Flags: review?(jcoppeard)
![]() |
Assignee | |
Updated•8 years ago
|
Summary: (part 1) - Use MOZ_CHECK more in jsnum.h → Use MOZ_CHECK more in SpiderMonkey
Updated•8 years ago
|
Attachment #8745198 -
Flags: review?(jcoppeard) → review+
![]() |
Assignee | |
Updated•8 years ago
|
Summary: Use MOZ_CHECK more in SpiderMonkey → Use MOZ_MUST_USE more in SpiderMonkey
![]() |
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.
![]() |
Assignee | |
Updated•8 years ago
|
Keywords: leave-open
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8745198 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Blocks: use-nodiscard
Comment 4•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e791431d1864
![]() |
Assignee | |
Comment 5•8 years ago
|
||
Attachment #8748965 -
Flags: review?(jcoppeard)
![]() |
Assignee | |
Comment 6•8 years ago
|
||
Attachment #8748970 -
Flags: review?(jcoppeard)
![]() |
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.
Attachment #8748987 -
Flags: review?(bbouvier)
![]() |
Assignee | |
Comment 8•8 years ago
|
||
Attachment #8748989 -
Flags: review?(sphink)
![]() |
Assignee | |
Comment 9•8 years ago
|
||
Attachment #8749005 -
Flags: review?(jorendorff)
Updated•8 years ago
|
Attachment #8748989 -
Flags: review?(sphink) → review+
![]() |
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.
Attachment #8749021 -
Flags: review?(jorendorff)
Updated•8 years ago
|
Attachment #8748965 -
Flags: review?(jcoppeard) → review+
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.
Attachment #8748970 -
Flags: review?(jcoppeard) → review+
Updated•8 years ago
|
Attachment #8749005 -
Flags: review?(jorendorff) → review+
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.
Attachment #8749021 -
Flags: review?(jorendorff) → review+
![]() |
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.
Attachment #8750166 -
Flags: feedback?(terrence)
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.
Attachment #8748987 -
Flags: review?(bbouvier) → review+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8748965 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8748970 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8748989 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8749005 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8749021 -
Flags: checkin+
![]() |
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.
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8748987 -
Flags: checkin+
![]() |
Assignee | |
Comment 16•8 years ago
|
||
Attachment #8750624 -
Flags: review?(hv1989)
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.
Attachment #8750624 -
Flags: review?(hv1989) → review+
![]() |
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...
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.
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
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.
Attachment #8750166 -
Flags: feedback?(terrence) → feedback+
![]() |
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.
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/47dcb8959eed
![]() |
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()
Attachment #8752002 -
Flags: review?(terrence)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8750166 -
Attachment is obsolete: true
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.
Attachment #8752002 -
Flags: review?(terrence) → review+
![]() |
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.
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/742dca602ca3
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8750624 -
Flags: checkin+
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8752002 -
Flags: checkin+
![]() |
Assignee | |
Comment 28•7 years ago
|
||
This patch fixes numerous unchecked calls.
Attachment #8758557 -
Flags: review?(terrence)
Comment 29•7 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!
Attachment #8758557 -
Flags: review?(terrence) → review+
![]() |
Assignee | |
Comment 30•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3c2afcc1d78153c4ace5b41e7c238c4d54ee8d Bug 1267551 (part 10) - Use MOZ_MUST_USE in AutoVectorRooterBase. r=terrence.
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8758557 -
Flags: checkin+
![]() |
Assignee | |
Comment 31•7 years ago
|
||
This catches a missing check.
Attachment #8759529 -
Flags: review?(hv1989)
Updated•7 years ago
|
Attachment #8759529 -
Flags: review?(hv1989) → review+
Comment 32•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac3c2afcc1d7
![]() |
Assignee | |
Comment 33•7 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.
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8759529 -
Flags: checkin+
![]() |
Assignee | |
Comment 34•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a87ed5532b9e49a8ada850d80b39ca12f512c5dc Bug 1267551 (part 11b) - Follow-up to fix Android bustage. r=me.
Comment 35•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc00bd9f0d09 https://hg.mozilla.org/mozilla-central/rev/a87ed5532b9e
![]() |
Assignee | |
Comment 36•7 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.
Attachment #8761426 -
Flags: review?(hv1989)
![]() |
Assignee | |
Comment 37•7 years ago
|
||
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 38•7 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();
Attachment #8761426 -
Flags: review?(hv1989) → review+
Comment 39•7 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; }
Updated•7 years ago
|
Attachment #8761428 -
Flags: review?(hv1989) → review+
![]() |
Assignee | |
Comment 40•7 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 :)
![]() |
Assignee | |
Comment 41•7 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.
Comment 42•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a8dac921f074
Comment 43•7 years ago
|
||
Attachment #8762389 -
Flags: review?(jimb)
Comment 44•7 years ago
|
||
Attachment #8762390 -
Flags: review?(jimb)
Comment 45•7 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...
Attachment #8762391 -
Flags: review?(jimb)
Comment 46•7 years ago
|
||
Attachment #8762392 -
Flags: review?(jimb)
Comment 47•7 years ago
|
||
Attachment #8762393 -
Flags: review?(jimb)
Comment 48•7 years ago
|
||
Attachment #8762394 -
Flags: review?(jimb)
Comment 49•7 years ago
|
||
Attachment #8762395 -
Flags: review?(jimb)
Comment 50•7 years ago
|
||
Attachment #8762396 -
Flags: review?(jimb)
Comment 51•7 years ago
|
||
Attachment #8762397 -
Flags: review?(jimb)
Comment 52•7 years ago
|
||
Attachment #8762398 -
Flags: review?(jimb)
Comment 53•7 years ago
|
||
Try push for my queue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d8a2e50a77
Comment 54•7 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...)
![]() |
Assignee | |
Comment 55•7 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?
Updated•7 years ago
|
Attachment #8762389 -
Flags: review?(jimb) → review+
Updated•7 years ago
|
Attachment #8762390 -
Flags: review?(jimb) → review+
Updated•7 years ago
|
Attachment #8762391 -
Flags: review?(jimb) → review+
Updated•7 years ago
|
Attachment #8762392 -
Flags: review?(jimb) → review+
Updated•7 years ago
|
Attachment #8762393 -
Flags: review?(jimb) → review+
Updated•7 years ago
|
Attachment #8762394 -
Flags: review?(jimb) → review+
Updated•7 years ago
|
Attachment #8762395 -
Flags: review?(jimb) → review+
Updated•7 years ago
|
Attachment #8762396 -
Flags: review?(jimb) → review+
Updated•7 years ago
|
Attachment #8762397 -
Flags: review?(jimb) → review+
Comment 56•7 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.
Attachment #8762398 -
Flags: review?(jimb) → review-
Comment 57•7 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
Comment 58•7 years ago
|
||
Ok, fixed the ordering with JS_PUBLIC_API.
Attachment #8763225 -
Flags: review?(jimb)
Updated•7 years ago
|
Attachment #8762398 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 59•7 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.
Updated•7 years ago
|
Attachment #8763225 -
Flags: review?(jimb)
Comment 61•7 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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•