Last Comment Bug 530299 - Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX in /xpcom
: Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX in /xpcom
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- minor (vote)
: ---
Assigned To: Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
:
Mentors:
Depends on:
Blocks: 518502
  Show dependency treegraph
 
Reported: 2009-11-21 06:59 PST by Jae-Seong Lee-Russo
Modified: 2011-07-06 15:53 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Search&Replace, 0 (19.60 KB, patch)
2009-11-21 07:01 PST, Jae-Seong Lee-Russo
no flags Details | Diff | Splinter Review
mozilla::min & mozilla::max (857 bytes, patch)
2009-11-23 23:10 PST, Jae-Seong Lee-Russo
no flags Details | Diff | Splinter Review
mozilla::min and max, rev. 1 (25.02 KB, patch)
2009-12-01 14:23 PST, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
cjones.bugs: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Jae-Seong Lee-Russo 2009-11-21 06:59:36 PST
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
Comment 1 Jae-Seong Lee-Russo 2009-11-21 07:01:37 PST
Created attachment 413816 [details] [diff] [review]
Search&Replace, 0
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-11-23 13:33:24 PST
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?
Comment 3 Jae-Seong Lee-Russo 2009-11-23 23:10:40 PST
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?
Comment 4 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-12-01 14:23:15 PST
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.
Comment 5 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2009-12-01 17:04:25 PST
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
Comment 6 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2009-12-01 17:05:17 PST
Also, is there any reason not to put this in xpcom/glue?
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2009-12-04 08:57:11 PST
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.
Comment 8 Ed Morley [:emorley] 2011-07-06 15:53:46 PDT
Fixed by bug 661584.

Note You need to log in before you can comment on or make changes to this bug.