Bug 83602

Created attachment 136513 [details] Patch

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

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

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

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

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.

Created attachment 137603 [details] Patch

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 on attachment 137603 [details] Patch Seams OK. I'm still surprised we need to solve this this way.

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

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

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.

(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 on attachment 137603 [details] Patch Clearing flags on attachment: 137603 Committed r114537: <http://trac.webkit.org/changeset/114537>

All reviewed patches have been landed. Closing bug.