If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX, in layout/

RESOLVED FIXED in mozilla1.9.3a1

Status

()

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

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
The PR_MIN/PR_MAX are macros that evaluate (one of) its arguments twice
which is error-prone and can be a performance cost.
I think we should switch to using NS_MIN/NS_MAX instead, in all C++ code
if we can, and then #undef these macros in a central location to protect
us from mistakes (like bug 511482).
(Assignee)

Comment 1

8 years ago
Created attachment 396104 [details] [diff] [review]
Part 0, automated changes

Just to show you what it would look like for all of layout/,
this part was generated by:
sed -i -e 's/PR_MAX/NS_MAX/g' -e 's/PR_MIN/NS_MIN/g'  $(egrep -lr 'PR_MAX|PR_MIN' layout/)
(Assignee)

Comment 2

8 years ago
Created attachment 396105 [details] [diff] [review]
Part 1, manual changes
Version: unspecified → Trunk
(Assignee)

Comment 3

8 years ago
dbaron> NS_MAX might actually have
dbaron> its own issues because it coerces both arguments
dbaron> to be the same type.

mats> ... isn't that actually a good thing?  Since it doesn't
mats> compile it forces the author to think about the types, and it's
mats> clearer to a casual reader?  An example:
mats> -      textRunSize = NS_MAX(textRunSize, CLAMP_MIN_SIZE);
mats> +      textRunSize = NS_MAX(textRunSize, double(CLAMP_MIN_SIZE));
mats>
mats> FYI, it wasn't very many places it occurred so shouldn't
mats> be a burden I think.

dbaron> Probably ok with me; might want to see what roc thinks.

roc?
Sounds OK to me.
(Assignee)

Updated

8 years ago
Assignee: nobody → matspal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 5

8 years ago
Created attachment 399342 [details] [diff] [review]
Part 0, automated changes

PR_MIN/MAX => NS_MIN/MAX (for layout/)
Attachment #396104 - Attachment is obsolete: true
Attachment #399342 - Flags: review?(roc)
(Assignee)

Comment 6

8 years ago
Created attachment 399343 [details] [diff] [review]
Part 1, manual changes

Manual changes:
  adding #include "nsAlgorithm.h" in a few places
  adding a few type conversions
Attachment #396105 - Attachment is obsolete: true
Attachment #399343 - Flags: review?(roc)
Attachment #399343 - Flags: review?(roc) → review+
(Assignee)

Comment 7

8 years ago
Comment on attachment 399342 [details] [diff] [review]
Part 0, automated changes

roc, rubber stamp please.
Attachment #399342 - Flags: review?(roc) → review+
m-c: "Too many hits, displaying the first 1000"

Is this bug actually for Layout only?
(Assignee)

Comment 9

8 years ago
(In reply to comment #8)
> m-c: "Too many hits, displaying the first 1000"
> 
> Is this bug actually for Layout only?

This bug only fixes files under layout/
I'll file separate bugs for the rest where it seems appropriate (code is C++).
(Assignee)

Comment 10

8 years ago
Created attachment 401036 [details] [diff] [review]
The final patch as landed

http://hg.mozilla.org/mozilla-central/rev/1348d3ce063d
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Blocks: 518502
(In reply to comment #9)
> I'll file separate bugs for the rest where it seems appropriate (code is C++).

I eventually filed bug 518502.
Summary: Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX → Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX, in layout/

Comment 12

8 years ago
Created attachment 413882 [details] [diff] [review]
/layout/tables/nsTableFrame.cpp, 0
Attachment #413882 - Flags: review?(fantasai.bugs)

Updated

8 years ago
Attachment #413882 - Flags: review?(fantasai.bugs) → review+
(In reply to comment #12)
> Created an attachment (id=413882) [details]
> /layout/tables/nsTableFrame.cpp, 0

Does this patch need to be checked-in now, or was it moved to another bug?
Blocks: 537142
(In reply to comment #0)
> I think we should switch to using NS_MIN/NS_MAX instead, in all C++ code
> if we can, and then #undef these macros in a central location to protect
> us from mistakes (like bug 511482).

Now that bug 661584 is complete, is the #undef part of your proposal in comment 0 still desired? NSPR still uses PR_(MAX|MIN|ABS|ROUNDUP) internally, so not sure where the best places is to #undef them for outside of NSPR only?
(Assignee)

Comment 15

6 years ago
Ed, I'm glad to see all these are gone now, thanks!

I don't think adding an #undef in a header somewhere will help much
since it needs to come after the #include of prtypes.h to have an effect.
The best would be to remove them from prtypes.h but I suspect we can't
do that.  We should simply stop using other things from that header file,
PRBool for example, and then eventually we can just remove the #include.

Also, I think the work that has been done to switch to NS_ has made many
people aware that the PR_ versions shouldn't be used.  Maybe a short note
to m.d.platform to spread the word would be a good idea?
(In reply to Mats Palmgren [:mats] from comment #0)
> #undef these macros in a central location

(In reply to Mats Palmgren [:mats] from comment #15)
> We should simply stop using other things from that header file,
> PRBool for example, and then eventually we can just remove the #include.

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