Closed Bug 512106 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug, )

Details

Attachments

(4 files, 2 obsolete files)

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).
Attached patch Part 0, automated changes (obsolete) — Splinter Review
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/)
Attached patch Part 1, manual changes (obsolete) — Splinter Review
Version: unspecified → Trunk
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?
Assignee: nobody → matspal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
PR_MIN/MAX => NS_MIN/MAX (for layout/)
Attachment #396104 - Attachment is obsolete: true
Attachment #399342 - Flags: review?(roc)
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)
Comment on attachment 399342 [details] [diff] [review]
Part 0, automated changes

roc, rubber stamp please.
m-c: "Too many hits, displaying the first 1000"

Is this bug actually for Layout only?
(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++).
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(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/
Attachment #413882 - Flags: review?(fantasai.bugs)
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?
(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?
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.