Bug 60638 – check-webkit-style should warn about types we normally pass as reference

• Status: REOPENED
• Component: Tools / Tests

[Eric Seidel (no email)]

check-webkit-style should warn about types we normally pass as reference

If check-webkit-style ever sees a IntSize or IntPoint passed by value, it should warn.  (I'm sure there are a bunch of others).  I can't think of any reason to no pass them by const &.

https://bugs.webkit.org/show_bug.cgi?id=60592#c6

[Darin Adler]

Despite the fact that I think this is probably a good guideline, there are at least two possible benefits to passing a type like IntPoint by value instead of by reference.

First, Anders Carlsson has claimed that in some situations on some platforms it may be more efficient. He can fill in the details at some point when he has the chance.

Second, if you pass a point by reference, you can run into cases where reentrancy can change the value.

Here's an example:

    scrollTo(m_documentSize)

    inside the scrollTo function

    FrameView::scrollTo(const IntPoint& position)
    {
        ... position has the value passed in ...
        ... do some work that changes the document size ...
        ... position is now the new document size ...
    }

This can mean that if you, say, check for a special value such as zero, the value can change later in the same function.

[Eric Seidel (no email)]

I'm happy to close this as invalid then.  Thank you for the background.

[Darin Adler]

Oh, I wasn’t saying this is invalid. I think the rule is probably a good one. I was just responding to your comment that there are *no* reasons to ever prefer, say, IntPoint, to const IntPoint&. We might still decide that for our project these are never compelling reasons to do so, and perhaps should.

[Eric Seidel (no email)]

OK.  Then sounds like we should come up with a list of types which we generally pass this way and add a rule to the style bot. :)

[Alexey Proskuryakov]

If passing an IntPoint by value would save a register on Mac OS X , then maybe we should just do that.

[Alexey Proskuryakov]

I guess my comment didn't make a lot of sense. It's just my feeling that passing by value would be better for performance on many platforms, but I'm not really an expert.

[Simon Fraser (smfr)]

> If passing an IntPoint by value would save a register on Mac OS X
We shouldn't forget 32-bit builds and ARM.

[David Levin]

It looks like people are being guided to using "const IntPoint&"

https://bugs.webkit.org/show_bug.cgi?id=60679#c5

Perhaps the guideline is if it begins with a capital letter (aka a custom defined type), then it is a pass by ref (const is optional) or pointer but not by value.

Then if there are exceptions to this rule we can special case those types. To say that they should be pass by value, but it sounds like IntPoint is not an exception.

[Levi Weintraub]

(In reply to comment #8)
> It looks like people are being guided to using "const IntPoint&"
> 
> https://bugs.webkit.org/show_bug.cgi?id=60679#c5
> 
> Perhaps the guideline is if it begins with a capital letter (aka a custom defined type), then it is a pass by ref (const is optional) or pointer but not by value.
> 
> Then if there are exceptions to this rule we can special case those types. To say that they should be pass by value, but it sounds like IntPoint is not an exception.

I'd just like to see a consensus so we can avoid butting heads on individual bugs when which is best could be argued.