372971 - Convert macros to inline functions

Status: RESOLVED WONTFIX

(Core :: General, defect)

Description

[(dormant account)]

Attachment: Initial patch that compiles and links

PR_MAX|MIN, and similar functions can cause unintended code expansion and make automated analysis more difficult.
Other macros contain large amounts of code and also prevent parsing tools from refactoring code.

Comment 1

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

Can't you define some additional PR_MIN/PR_MAX overloads so that (float,int) combinations work without requiring caller changes?

Comment 2

[(dormant account)]

Attachment: Overloaded min/max functions

The patch is a bit smaller after providing the overloads. It would be even smaller if there were overloads to mix signed/unsigned types, but that seems like a bad idea.

Comment 3

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

Context diff please?

Comment 4

[(dormant account)]

Attachment: Overloaded min|max. used -u this time

Comment 5

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

Use constructor-style casts instead of C-style casts.

+        PRInt32 beginIn = PR_MAX(0ll, blockOffset - p->mOffset);
+        PRInt32 beginOut = PR_MAX(0ll, p->mOffset - blockOffset);

Could you use constructor casts here instead? 0ll looks really strange to me and looks a lot like 011.

The only other thing I'm worried about is whether we can change stuff in nsprpub. I'm not sure who owns that actually.

Comment 6

[Brendan Eich [:brendan]]

0LL is more legible.

/be

Comment 7

[(dormant account)]

I still don't have a good idea on how to deal with the
NS_IMPL_HTMLBODY_COLOR_ATTR macro in nsHtmlBodyElement.cpp. Should we just
leave it and hope that squash doesn't run into problems with it?

Comment 8

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

It gets used 4 times. Why don't you just create the 8 generated functions explicitly. For the Get versions, you can use a helper function that takes the attribute nsIAtom* and the default nsColor, so each Get implementation is just
NS_IMETHODIMP
nsHTMLBodyElement::GetVLink(nsAString& aColor)
{
  return GetColorHelper(nsGkAtoms::vlink, GetPresContext()->DefaultVisitedLinkColor(), aColor);
}

Comment 9

[Wan-Teh Chang]

Comment on attachment 257712 [details] [diff] [review]
Overloaded min|max. used -u this time

If this patch requires adding the typecasts to some of the arguments
to PR_MAX and PR_MIN, this will break "source compatibility".

Many products other than the Mozilla clients use NSPR, and some of
them are also C++ applications.  Could you come up with a patch
that maintain source compatibility?

Comment 10

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

That's not really possible as far as I know. Can we have some kind of configure option that enables this feature for Mozilla but is off by default?

Comment 11

[(dormant account)]

Attachment: overloaded & static casting & PR_ABS & using moz typedefs

Use 0LL instead of 0ll, static casts.
Changed PR_ABS to be a function too.
Using typedefs instead of underlying types in the definitions.

Comment 12

[(dormant account)]

(In reply to comment #10)
> That's not really possible as far as I know. Can we have some kind of configure
> option that enables this feature for Mozilla but is off by default?
> 

Or we could change PR_MAX|MIN in the source code to call a function with a new name instead... Such as the std:: C++ one. But I guess that's more of a Mozilla 2 scope.

Comment 13

[(dormant account)]

Attachment: nsHTMLBodyElement macro expansion

Not sure if the helper method needs to be private too.

Comment 14

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

Comment on attachment 257776 [details] [diff] [review]
nsHTMLBodyElement macro expansion

+  if (!aElement->GetAttr(kNameSpaceID_None, aAtom, color)) {
+      NS_RGBToHex(defaultAttrColor, aColor);

Fix indent

Make GetColorHelper just be a nonvirtual method that returns nsresult, not an NS_IMETHOD (that makes it virtual which is unnecessary). Yes, make it private too.

Comment 15

[(dormant account)]

Attachment: nsHTMLBodyElement macro expansion #2

Fixed indent, changed signature. I accidentally left an extra parameter in the last patch, that's gone now.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Landed attachment 257778 [details] [diff] [review] on the trunk.
mozilla/content/html/content/src/nsHTMLBodyElement.cpp 	1.162

Comment 17

[(dormant account)]

Attachment: missing null check

Missed a null check

Comment 18

[(dormant account)]

Comment on attachment 258470 [details] [diff] [review]
missing null check

This is very wrong

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Probably not relevant anymore (and the wrong component)