Bug 107227 – Fix WTF::copyLCharsFromUCharSource() to compile with -Wshorten-64-to-32

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

<http://webkit.org/b/000000>

Reviewed by NOBODY (OOPS!).

Fixes the following build error:

ASCIIFastPath.h:117:59: error: implicit conversion loses integer precision: 'unsigned long' to 'const unsigned int' [-Werror,-Wshorten-64-to-32]
const unsigned endLength = length - ucharsPerLoop + 1;
~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~^~~
1 error generated.

* wtf/text/ASCIIFastPath.h:
(WTF::copyLCharsFromUCharSource): Change local variables from
unsigned to size_t.
---
 2 files changed, 20 insertions(+), 2 deletions(-)

[David Kilzer (:ddkilzer)]

Created attachment 183374 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 183374 [details]
Patch

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

> Source/WTF/ChangeLog:4
> +        Fix WTF::copyLCharsFromUCharSource() to compile with -Wshorten-64-to-32
> +        <107227>

Oops!  Trying to make webkit-patch replace bug ID placeholders in my patches.
My re.sub() fu is weak!

[David Kilzer (:ddkilzer)]

Created attachment 183376 [details]
Patch v2

[Benjamin Poulain]

Comment on attachment 183376 [details]
Patch v2

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

> Source/WTF/wtf/text/ASCIIFastPath.h:115
>      const uintptr_t sourceLoadSize = 32; // Process 32 bytes (16 UChars) each iteration
> -    const unsigned ucharsPerLoop = sourceLoadSize / sizeof(UChar);
> +    const size_t ucharsPerLoop = sourceLoadSize / sizeof(UChar);

You may want to change sourceLoadSize to size_t for the case where sizeof(size_t) < sizeof(void*)

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> (From update of attachment 183376 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183376&action=review
> 
> > Source/WTF/wtf/text/ASCIIFastPath.h:115
> >      const uintptr_t sourceLoadSize = 32; // Process 32 bytes (16 UChars) each iteration
> > -    const unsigned ucharsPerLoop = sourceLoadSize / sizeof(UChar);
> > +    const size_t ucharsPerLoop = sourceLoadSize / sizeof(UChar);
> 
> You may want to change sourceLoadSize to size_t for the case where sizeof(size_t) < sizeof(void*)

On what systems would the size of size_t be smaller than the size of a pointer?  I can't think of any off the top of my head, but it's late and I haven't worked with any "interesting" systems recently.  :)

This code is also inside this macro:

#if OS(DARWIN) && (CPU(X86) || CPU(X86_64))

So I doubt uintptr_t and size_t are going to change sizes in those configurations.

Looking at the code, though, I guess it would be a good idea to use size_t just to match the other local variables.  Any reason why uintptr_t was chosen here to begin with?

[Benjamin Poulain]

> On what systems would the size of size_t be smaller than the size of a pointer?  I can't think of any off the top of my head, but it's late and I haven't worked with any "interesting" systems recently.  :)

I can just be wrong, but I am thinking about mixed ABI like x32.

Although, I guess it is the other way around then and sizeof(size_t) > sizeof(void*).

> This code is also inside this macro:
> 
> #if OS(DARWIN) && (CPU(X86) || CPU(X86_64))
>
> So I doubt uintptr_t and size_t are going to change sizes in those configurations.

Right! I missed that. Yep, that does not matter in this case.

> Looking at the code, though, I guess it would be a good idea to use size_t just to match the other local variables.  Any reason why uintptr_t was chosen here to begin with?

I think it was just to have a type semantically correct to explain the "/ size(...)" later.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> > Looking at the code, though, I guess it would be a good idea to use size_t just to match the other local variables.  Any reason why uintptr_t was chosen here to begin with?
> 
> I think it was just to have a type semantically correct to explain the "/ size(...)" later.

On second thought, I think I'll leave it unchanged for now.  It can always be fixed later if it's an issue.

Thanks!

[David Kilzer (:ddkilzer)]

Committed r140162: <http://trac.webkit.org/changeset/140162>

[Darin Adler]

Comment on attachment 183376 [details]
Patch v2

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

>>> Source/WTF/wtf/text/ASCIIFastPath.h:115
>>> +    const size_t ucharsPerLoop = sourceLoadSize / sizeof(UChar);
>> 
>> You may want to change sourceLoadSize to size_t for the case where sizeof(size_t) < sizeof(void*)
> 
> On what systems would the size of size_t be smaller than the size of a pointer?  I can't think of any off the top of my head, but it's late and I haven't worked with any "interesting" systems recently.  :)
> 
> This code is also inside this macro:
> 
> #if OS(DARWIN) && (CPU(X86) || CPU(X86_64))
> 
> So I doubt uintptr_t and size_t are going to change sizes in those configurations.
> 
> Looking at the code, though, I guess it would be a good idea to use size_t just to match the other local variables.  Any reason why uintptr_t was chosen here to begin with?

Agreed that it should be size_t just for consistency. But not sure that it earns its keep as a named variable, really, since it’s just used in the line below. I think that comment would be clearer if it was on a line by itself and the “32” could just be in the line below; the comment is a great one for the ucharsPerLoop initialization.