Bug 83602

Summary: Use explicit casts for size_t to unsigned conversion
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, jchaffraix, leviw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch none

Description Emil A Eklund 2012-04-10 12:08:21 PDT
Not all platforms have implicit casts from size_t to unsigned and we can't add size_t versions of the operators to FractionalLayoutUnit for all platforms as it would conflict with the unsigned versions of same.
Comment 1 Emil A Eklund 2012-04-10 12:34:48 PDT
Created attachment 136513 [details]
Patch
Comment 2 Julien Chaffraix 2012-04-11 16:54:44 PDT
Comment on attachment 136513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136513&action=review

> Source/WebCore/ChangeLog:10
> +        Not all platforms have implicit casts from size_t to unsigned and we
> +        can't add size_t versions of the operators to FractionalLayoutUnit for
> +        all platforms as it would conflict with the unsigned versions of same.

I may not understand everything but Levi was mentioning that on some platforms, size_t and unsigned are the same type which was the issue you had. In this case, wouldn't it be easier to just #ifdef those operators on the right platforms?

We haven't done a good job at maintaining the distinction between size_t and unsigned (I have to plead guilty on this one), so I wonder how many more call sites would need to be updated? Shouldn't we reach consensus before making the code use unsigned instead of size_t (or the other way around)?
Comment 3 Emil A Eklund 2012-04-11 17:01:38 PDT
(In reply to comment #2)
> (From update of attachment 136513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136513&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Not all platforms have implicit casts from size_t to unsigned and we
> > +        can't add size_t versions of the operators to FractionalLayoutUnit for
> > +        all platforms as it would conflict with the unsigned versions of same.
> 
> I may not understand everything but Levi was mentioning that on some platforms, size_t and unsigned are the same type which was the issue you had. In this case, wouldn't it be easier to just #ifdef those operators on the right platforms?

That would be the other option. 

> We haven't done a good job at maintaining the distinction between size_t and unsigned (I have to plead guilty on this one), so I wonder how many more call sites would need to be updated? Shouldn't we reach consensus before making the code use unsigned instead of size_t (or the other way around)?

We only really seem to use size_t in a handful of places and there doesn't seem to be any standard way of comparing/converting size_t, int and unsigned values.

If you think it's better to add size_t versions of the methods to FractionalLayoutUnit for the platforms where it doesn't typedef to undefined I'll gladly make the change.
Comment 4 Julien Chaffraix 2012-04-11 17:28:56 PDT
> > We haven't done a good job at maintaining the distinction between size_t and unsigned (I have to plead guilty on this one), so I wonder how many more call sites would need to be updated? Shouldn't we reach consensus before making the code use unsigned instead of size_t (or the other way around)?
> 
> We only really seem to use size_t in a handful of places and there doesn't seem to be any standard way of comparing/converting size_t, int and unsigned values.

I was talking globally over WebCore. I don't think there is any consensus on whether we should use size_t vs unsigned in the code (for iterating over Vectors a quick grep shows that it's pretty divided).

> If you think it's better to add size_t versions of the methods to FractionalLayoutUnit for the platforms where it doesn't typedef to undefined I'll gladly make the change.

It really sounds less intrusive to add the overloaded functions for size_t. It's also less fragile (anyone could come before - or after - you land the final patch and break everything). I am assuming that you only need a small number of overloaded functions though.
Comment 5 Emil A Eklund 2012-04-11 18:01:28 PDT
(In reply to comment #4)
> It really sounds less intrusive to add the overloaded functions for size_t. It's also less fragile (anyone could come before - or after - you land the final patch and break everything). I am assuming that you only need a small number of overloaded functions though.

Sure, I was more worried about maintaining the ifdefs needed but in reality we don't add new platforms very often so it probably makes more sense to do it that way.

Thanks for your feedback!
Comment 6 Emil A Eklund 2012-04-17 14:04:27 PDT
In light of the difficulties involved with getting even the same platform to agree on whether size_t and unsigned are interchangeable this approach seems both safer and more sense.
Comment 7 Emil A Eklund 2012-04-17 14:32:13 PDT
Created attachment 137603 [details]
Patch
Comment 8 Emil A Eklund 2012-04-18 09:46:47 PDT
Any thoughts on this? I tried to make it cleaner by changing a couple of the functions to take unsigned instead of size_t, thereby reducing the number of explicit casts needed.
Comment 9 Eric Seidel (no email) 2012-04-18 10:34:35 PDT
Comment on attachment 137603 [details]
Patch

Seams OK.  I'm still surprised we need to solve this this way.
Comment 10 Eric Seidel (no email) 2012-04-18 10:36:13 PDT
Comment on attachment 137603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137603&action=review

> Source/WebCore/ChangeLog:12
> +        Not all platforms have implicit casts from size_t to unsigned and we
> +        can't add size_t versions of the operators to FractionalLayoutUnit for
> +        all platforms as it would conflict with the unsigned versions of same.

I still would think we could do this with a compile guard, like #if sizeof(unsigned) == sizeof(size_t)?  I guess sizeof isn't enough though...
Comment 11 Emil A Eklund 2012-04-18 10:39:01 PDT
Comment on attachment 137603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137603&action=review

>> Source/WebCore/ChangeLog:12
>> +        all platforms as it would conflict with the unsigned versions of same.
> 
> I still would think we could do this with a compile guard, like #if sizeof(unsigned) == sizeof(size_t)?  I guess sizeof isn't enough though...

I thought so as well but no joy. Neither did checking the memory model (LLP64 vs LP64 vs 32-bit). The check builds (valgrind) where did real complication and I could not find a way around that.
Comment 12 Eric Seidel (no email) 2012-04-18 10:43:48 PDT
Comment on attachment 137603 [details]
Patch

We could alos come up with a blacklist of compilers and set some #define in Platform.h... That blacklist might be difficutl to come up with, but seems more long-term useful than disallowing size_t in some parts of the WebKit code. :)
Comment 13 Emil A Eklund 2012-04-18 10:45:48 PDT
(In reply to comment #12)
> (From update of attachment 137603 [details])
> We could alos come up with a blacklist of compilers and set some #define in Platform.h... That blacklist might be difficutl to come up with, but seems more long-term useful than disallowing size_t in some parts of the WebKit code. :)

Given the number of platforms involved that doesn't seem practical. I agree that it would be nice to allow size_ts to be used in layout code though.
Comment 14 Eric Seidel (no email) 2012-04-18 10:48:06 PDT
We might at least add a FIXME to the FractionalLayoutUnit code noting this problem with size_t/unsigned and linking to this bug for future historians.  I'm not sure how common it will be that someone will add some size_t usage in the RenderingTree and try to shave this yak.  Having quick access to this bug discussion might at least inform them of what you've learned.
Comment 15 Emil A Eklund 2012-04-18 11:28:38 PDT
(In reply to comment #14)
> We might at least add a FIXME to the FractionalLayoutUnit code noting this problem with size_t/unsigned and linking to this bug for future historians.  I'm not sure how common it will be that someone will add some size_t usage in the RenderingTree and try to shave this yak.  Having quick access to this bug discussion might at least inform them of what you've learned.

Good idea. Will do.
Comment 16 WebKit Review Bot 2012-04-18 11:31:58 PDT
Comment on attachment 137603 [details]
Patch

Clearing flags on attachment: 137603

Committed r114537: <http://trac.webkit.org/changeset/114537>
Comment 17 WebKit Review Bot 2012-04-18 11:32:03 PDT
All reviewed patches have been landed.  Closing bug.