NS_MIN/NS_MAX are useless dupes of std::min / std::max

RESOLVED FIXED in mozilla21

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: mats)

Tracking

unspecified
mozilla21
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

They even work in the same way, being function templates. Why have our own? It's just one more thing to learn for new contributors and one more thing to tweak when importing/sharing code.

Comment 1

5 years ago
I filed bug 786533 to convert NS_MIN to std::min.

See the discussion in bug 785542 (comment 2 through 5) for why NS_MAX and std::max are different.  I'm not quite sure how we can solve that.  Mats suggested converting NS_MAX(x, y) to std::max(y, x).  But that's sort of fragile in case in the future we compile with a compiler which gets this right.  We could add a configure check and have our own wrapper around std::max which always does the right thing, but that defeats the purpose.  Or we could just fail configure on platforms which have the NS_MAX behavior for std::max, and address the problem when we cross that bridge.

Thoughts?
Component: XPCOM → MFBT
From bug 785542 comment 6, the specification requires that std::min/std::max return the first argument if both are equal.

NS_MAX is wrong according to the spec.
(Assignee)

Comment 3

5 years ago
Note also gfx_min/max, which has the same problem as NS_MIN/MAX:
http://mxr.mozilla.org/mozilla-central/source/gfx/2d/BaseRect.h#15

Comment 4

5 years ago
There's nothing wrong about NS_MAX, it was never designed to be a standard library function.  ;-)
This can be closed now that bug 786533 is fixed, right?
(Assignee)

Comment 6

5 years ago
Yes, bug 786533 fixed this.
Assignee: nobody → matspal
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.