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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

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).

Comment 1

[Mats Palmgren (inactive)]

Attachment: 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/)

Comment 2

[Mats Palmgren (inactive)]

Attachment: Part 1, manual changes

Comment 3

[Mats Palmgren (inactive)]

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?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds OK to me.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Part 0, automated changes

PR_MIN/MAX => NS_MIN/MAX (for layout/)

Comment 6

[Mats Palmgren (inactive)]

Attachment: Part 1, manual changes

Manual changes:
  adding #include "nsAlgorithm.h" in a few places
  adding a few type conversions

Comment 7

[Mats Palmgren (inactive)]

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

roc, rubber stamp please.

Comment 8

[Serge Gautherie (:sgautherie)]

m-c: "Too many hits, displaying the first 1000"

Is this bug actually for Layout only?

Comment 9

[Mats Palmgren (inactive)]

(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++).

Comment 10

[Mats Palmgren (inactive)]

Attachment: The final patch as landed

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

Comment 11

[Serge Gautherie (:sgautherie)]

(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.

Comment 12

[Jae-Seong Lee-Russo]

Attachment: /layout/tables/nsTableFrame.cpp, 0

Comment 13

[Nochum Sossonko [:Natch]]

(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?

Comment 14

[Ed Morley [:emorley]]

(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?

Comment 15

[Mats Palmgren (inactive)]

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?

Comment 16

[Serge Gautherie (:sgautherie)]

(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.