Closed Bug 1527225 Opened 9 months ago Closed 9 months ago

Use mozilla::Clamp in Nursery sizing calculations

Categories

(Core :: JavaScript: GC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: pbone, Assigned: pbone)

Details

Attachments

(2 files)

See

https://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#1177,1182

This could be made more readable with a JS_CLAMP macro.

See mozilla::Clamp: https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/mfbt/MathAlgorithms.h#494

(In general don't add new macros where inline functions suffice.)

(In reply to Jan de Mooij [:jandem] from comment #1)

See mozilla::Clamp: https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/mfbt/MathAlgorithms.h#494

(In general don't add new macros where inline functions suffice.)

Oh thanks. yeah, I realised that sfink (in a code review) was hinting at this also. But I don't think either of us realised that it was already defined as mozilla::Clamp.

Thanks.

Assignee: nobody → pbone
Status: NEW → ASSIGNED
Summary: Introduce JS_CLAMP and use it in Nursery sizing calculations → Use mozilla::Clamp in Nursery sizing calculations

(In reply to Jan de Mooij [:jandem] from comment #1)

See mozilla::Clamp: https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/mfbt/MathAlgorithms.h#494

Ah, thanks! Yeah, I looked for it next to Min()/Max() at https://searchfox.org/mozilla-central/source/mfbt/TemplateLib.h#31

Should that be merged into MathAlgorithms.h?

Flags: needinfo?(jwalden)
Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43baecd33ce6
Use mozilla::Clamp in maybeResizeNursery() r=jandem

Thanks cbrindusan,

Turns out when I tested this I had other patches with it and those other patches fixed these problems. My bad.

Flags: needinfo?(pbone)
Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/632b98194776
Use mozilla::Clamp in maybeResizeNursery() r=sfink
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

Ah, thanks! Yeah, I looked for it next to Min()/Max() at https://searchfox.org/mozilla-central/source/mfbt/TemplateLib.h#31

Should that be merged into MathAlgorithms.h?

Yeah, those probably could stand to be moved over. "template lib" is a bit of a bad name, really, but it was the fast'n'easy choice back in the day. Mind spinning up a bug/patch for it?

Flags: needinfo?(jwalden)
You need to log in before you can comment on or make changes to this bug.