Bug 80926 – Fix some compiler warnings (miscellaneous)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[George Staikos]

Created attachment 131491 [details]
Fix signed/unsigned mismatches

+++ This bug was initially created as a clone of Bug #80790 +++

Patches to come - mostly signed/unsigned changes.

[Alexey Proskuryakov]

CC'ing some JSC folks in case these warnings point to something more interesting than just a need to cast.

[Alexey Proskuryakov]

Comment on attachment 131491 [details]
Fix signed/unsigned mismatches

As mentioned in original bug, please use "int" instead of "signed", and please use C++ casts.

[George Staikos]

Created attachment 131674 [details]
Don't define the static if it isn't used (ie: SA_RESTART defined)

[George Staikos]

(In reply to comment #2)
> (From update of attachment 131491 [details])
> As mentioned in original bug, please use "int" instead of "signed", and please use C++ casts.

http://www.cplusplus.com/doc/tutorial/typecasting/

Those are C++ casts.  I can change to int.

[George Staikos]

Created attachment 131678 [details]
Fix signed/unsigned mismatches

[Alexey Proskuryakov]

> Those are C++ casts.  I can change to int.

It's a long-standing and agreed upon rule in WebKit, although it doesn't seem to be mentioned in <http://www.webkit.org/coding/coding-style.html>. Perhaps because it's more of safe coding rule than a style one.

There is no doubt that C-style casts are still supported in C++, but that doesn't make them welcome in WebKit.

[Alexey Proskuryakov]

Comment on attachment 131678 [details]
Fix signed/unsigned mismatches

Still wrong casts.

[George Staikos]

(In reply to comment #7)
> (From update of attachment 131678 [details])
> Still wrong casts.

Please enlighten me.

[Alexey Proskuryakov]

> Please enlighten me.

Does comment 6 not help?

[George Staikos]

(In reply to comment #9)
> > Please enlighten me.
> 
> Does comment 6 not help?

Those are not C-style casts.  They're C++-style.  If you want a different C++ cast, please select:

1) static_cast<>
2) reinterpret_cast<>
3) dynamic_cast<> (of course it will fail, as will const_cast<>)

Really, they're c++ casts.  They also pass the style checker and the style guideline referenced.
http://www.cplusplus.com/doc/tutorial/typecasting/ explains.  I also think it looks a LOT nicer than an unneeded big long string of text.

Saying "use C++ cast" is meaningless since it is a C++ cast - and that's my point.

[Alexey Proskuryakov]

If you want WebKit coding style to change, please e-mail webkit-dev. If you can suggest a more clear description for C++ style casts to use in future reviews, please do so.

I think that you understood my comment from the first time, and are just trying to be difficult.

[George Staikos]

(In reply to comment #11)
> If you want WebKit coding style to change, please e-mail webkit-dev. If you can suggest a more clear description for C++ style casts to use in future reviews, please do so.
> 
> I think that you understood my comment from the first time, and are just trying to be difficult.

No, I want you to explain it.

[Filip Pizlo]

Comment on attachment 131678 [details]
Fix signed/unsigned mismatches

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

I agree with you that it is really enjoyable to have philosophical discussions about which of the various styles of casting to use.  There are many of them and their differences are fabulous.

However, what's important for us is that we stick to one style.  We use the foo_cast<bar>(baz) style, and we generally stay away from bar(baz) or (bar)baz.  That's just our convention.

> Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:218
> +        if (isForced || 5 * unsigned(m_numConsts) > 3 * maxPoolSize / sizeof(uint32_t))

This should be static_cast<m_numConsts>

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:463
> +            ARMWord tmp = (right.m_value == int(0x80000000)) ? ARMAssembler::INVALID_IMM : m_assembler.getOp2(-right.m_value);

This should be static_cast<int>(0x80000000).

> Source/JavaScriptCore/parser/Lexer.cpp:632
> +    ASSERT(unsigned(c) <= USHRT_MAX);

This should be static_cast<unsigned>(c)

[Filip Pizlo]

Comment on attachment 131678 [details]
Fix signed/unsigned mismatches

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

>> Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:218
>> +        if (isForced || 5 * unsigned(m_numConsts) > 3 * maxPoolSize / sizeof(uint32_t))
> 
> This should be static_cast<m_numConsts>

Correction, shjould be static_cast<unsigned>(m_numConstants)

[George Staikos]

(In reply to comment #13)
> (From update of attachment 131678 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131678&action=review
> 
> I agree with you that it is really enjoyable to have philosophical discussions about which of the various styles of casting to use.  There are many of them and their differences are fabulous.
> 
> However, what's important for us is that we stick to one style.  We use the foo_cast<bar>(baz) style, and we generally stay away from bar(baz) or (bar)baz.  That's just our convention.
> 
> > Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:218
> > +        if (isForced || 5 * unsigned(m_numConsts) > 3 * maxPoolSize / sizeof(uint32_t))
> 
> This should be static_cast<m_numConsts>

So in reality this has nothing to do with C++, but more "write it my way".  That's fine.  Let's just call it such.

[WebKit Review Bot]

Comment on attachment 131674 [details]
Don't define the static if it isn't used (ie: SA_RESTART defined)

Clearing flags on attachment: 131674

Committed r110617: <http://trac.webkit.org/changeset/110617>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 131678 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=131678&action=review
> > 
> > I agree with you that it is really enjoyable to have philosophical discussions about which of the various styles of casting to use.  There are many of them and their differences are fabulous.
> > 
> > However, what's important for us is that we stick to one style.  We use the foo_cast<bar>(baz) style, and we generally stay away from bar(baz) or (bar)baz.  That's just our convention.
> > 
> > > Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:218
> > > +        if (isForced || 5 * unsigned(m_numConsts) > 3 * maxPoolSize / sizeof(uint32_t))
> > 
> > This should be static_cast<m_numConsts>
> 
> So in reality this has nothing to do with C++, but more "write it my way".  That's fine.  Let's just call it such.

To be clear, it's about consistency.  It's better to have all of the code do it one way.  It may be the wrong way; who knows.  Doesn't really matter, since the semantics differences are minor.  I prefer (bar)baz, you prefer bar(baz), but the code already uses foo_cast<bar>(baz) everywhere so it's better if we stick to it.

[Nikolas Zimmermann]

(In reply to comment #15)
> So in reality this has nothing to do with C++, but more "write it my way".  That's fine.  Let's just call it such.

Indeed, it's really just our style. I've been yelled at the past in my patches, to stop using unsigned(foo) and use static_cast<unsigned>(foo) instead, always with the argument that C style casts should be avoided.
I was under the impression that (unsigned) foo and unsigned(foo) are C++ style casts, and it almost loos like Alexey is/was under the same impression.

We should nail this down and force static_cast<unsigned>() usage in the stye guide, to avoid confusion.

[Adam Treat]

(In reply to comment #19)
> (In reply to comment #15)
> > So in reality this has nothing to do with C++, but more "write it my way".  That's fine.  Let's just call it such.
> 
> Indeed, it's really just our style. I've been yelled at the past in my patches, to stop using unsigned(foo) and use static_cast<unsigned>(foo) instead, always with the argument that C style casts should be avoided.
> I was under the impression that (unsigned) foo and unsigned(foo) are C++ style casts, and it almost loos like Alexey is/was under the same impression.
> 
> We should nail this down and force static_cast<unsigned>() usage in the stye guide, to avoid confusion.

Or quit trying to enforce unwritten style guide rules.  I see *no* reason that the style guide should be updated to include this unwritten and informal 'style guideline'.

And arguing consistency... does this really make the code more readable/hackable?  Not at all in my opinion.  And that is what consistency is for -> to make the code more readable/hackable/followable.

[Alexey Proskuryakov]

It's incorrect that this is just for consistency. C-style casts (whether you write them using old C syntax "(type)variable" or constructor-style syntax "type(variable)" that you like to call C++ despite it having the same semantics) are more powerful and thus error-prone.

Again, if you want to change WebKit coding style, please go to webkit-dev.

[Adam Treat]

(In reply to comment #21)
> It's incorrect that this is just for consistency. C-style casts (whether you write them using old C syntax "(type)variable" or constructor-style syntax "type(variable)" that you like to call C++ despite it having the same semantics) are more powerful and thus error-prone.
> 
> Again, if you want to change WebKit coding style, please go to webkit-dev.

But I'm not trying to change webkit coding style: http://www.webkit.org/coding/coding-style.html

If this is not for consistency, then please tell me how the casts in this patch were error prone.  What future change do you think was likely to have an error because of this usage.