Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX in /xpcom

RESOLVED FIXED

Status

()

Core
XPCOM
--
minor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Jae-Seong Lee-Russo, Assigned: bsmedberg)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091120 Minefield/3.7a1pre (.NET CLR 3.5.30729)
Build Identifier: 

Please see Bug #512106.

Reproducible: Always
(Reporter)

Updated

8 years ago
Blocks: 518502
Version: unspecified → Trunk
(Reporter)

Comment 1

8 years ago
Created attachment 413816 [details] [diff] [review]
Search&Replace, 0
Attachment #413816 - Flags: review?(benjamin)
Assignee: nobody → lusian
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 2

8 years ago
I'm not especially happy with the names NS_MIN and NS_MAX when they aren't macros. I suppose that we can't just use std::min and std::max because apparently Windows.h defines min/max macros? Have you tried making mozilla::min and mozilla::max and seeing whether that compiles?
(Reporter)

Comment 3

8 years ago
Created attachment 414206 [details] [diff] [review]
mozilla::min & mozilla::max

I could compile /xpcom after applying this patch and replacing PR_MIN/PR_MAX with mozilla::min/mozilla::max, but is this patch correct?
Attachment #414206 - Flags: review?(benjamin)
(Assignee)

Comment 4

8 years ago
Created attachment 415492 [details] [diff] [review]
mozilla::min and max, rev. 1

Thanks for the patch. I've created an alternate version which uses a new header file in the style for new code.
Assignee: lusian → benjamin
Attachment #413816 - Attachment is obsolete: true
Attachment #414206 - Attachment is obsolete: true
Attachment #415492 - Flags: superreview?(dbaron)
Attachment #415492 - Flags: review?(jones.chris.g)
Attachment #413816 - Flags: review?(benjamin)
Attachment #414206 - Flags: review?(benjamin)
Comment on attachment 415492 [details] [diff] [review]
mozilla::min and max, rev. 1

>+#ifdef max
>+#undef max
>+#endif
>+#ifdef min
>+#undef min
>+#endif
>+
>+#define NOMINMAX

Could these have comments explaining what headers they're working around problems with?

>+const T&
>+min(const T& a, const T& b)
>+{
>+  return b < a ? b : a;

...
>+const T&
>+max(const T& a, const T& b)
>+{
>+  return a > b ? a : b;

So it's possible to swap the results of this expression by swapping any one of three things:
 * < vs >
 * a vs. b before the ?
 * a vs. b after the ?
It might make things a tad more readable if these two differed by only one of the three instead of differing by all 3.  Then again, it's easy enough to check that they're correct...

sr=dbaron
Attachment #415492 - Flags: superreview?(dbaron) → superreview+
Also, is there any reason not to put this in xpcom/glue?
Comment on attachment 415492 [details] [diff] [review]
mozilla::min and max, rev. 1

>diff --git a/xpcom/ds/algorithm.h b/xpcom/ds/algorithm.h
>new file mode 100644
>--- /dev/null
>+++ b/xpcom/ds/algorithm.h
>
>+template <class T>
>+inline
>+const T&
>+max(const T& a, const T& b)
>+{
>+  return a > b ? a : b;
>+}
>+

If you use |b < a| for this comparison, then T only needs to define operator<() to use both min and max.

r+ with that change.
Attachment #415492 - Flags: review?(jones.chris.g) → review+
Fixed by bug 661584.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.