Closed Bug 1472126 Opened Last year Closed Last year

Remove nullptr JSContext* check from NativeIterator::allocateSentinel() because it's never called with a nulllptr

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: anba, Assigned: khyperia)

Details

Attachments

(1 file, 2 obsolete files)

These lines [1] can be simplied to:
---
if (!ni)
    ReportOutOfMemory(maybecx);
---

because the |JSContext*| parameter is never a nullptr. And then the parameter name should be changed from |maybecx| to just |cx|.


[1] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/vm/Iteration.cpp#650-653
Priority: -- → P3
Assignee: nobody → khyperia
Status: NEW → ASSIGNED
I believe that the only reference to NativeIterator::allocateSentinel is in ObjectRealm::init [1]. (It's non-null.)


[1] https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/js/src/vm/Realm.cpp#92
Comment on attachment 9000083 [details] [diff] [review]
require non-null context in NativeIterator::allocateSentinel

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

Looks good, thanks.

If you post a new patch with r=jandem at the end of the commit message and the style nit below addressed, you can set the r+ flag yourself and add the checkin-needed keyword to the bug so sheriffs will land it. Or request r? if you want another review :)

::: js/src/vm/Iteration.cpp
@@ +660,4 @@
>  {
>      NativeIterator* ni = js_new<NativeIterator>();
>      if (!ni) {
> +        ReportOutOfMemory(cx);

Nit: you can remove the {} braces per our coding style [0] (or, maybe better, keep them and add |return nullptr;| on the next line) and we can also rename maybecx to cx in the Iteration.h header file.

https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style
Attachment #9000083 - Flags: review+
Attached patch Fix style nit (obsolete) — Splinter Review
Attachment #9000083 - Attachment is obsolete: true
Attachment #9001299 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdea655c3190
Require non-null context in NativeIterator::allocateSentinel. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bdea655c3190
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.