Bug 80926 - Fix some compiler warnings (miscellaneous)
Summary: Fix some compiler warnings (miscellaneous)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: George Staikos
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-12 19:13 PDT by George Staikos
Modified: 2012-03-14 10:35 PDT (History)
9 users (show)

See Also:


Attachments
Fix signed/unsigned mismatches (2.71 KB, patch)
2012-03-12 19:13 PDT, George Staikos
ap: commit-queue-
Details | Formatted Diff | Diff
Don't define the static if it isn't used (ie: SA_RESTART defined) (1.28 KB, patch)
2012-03-13 11:16 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Fix signed/unsigned mismatches (2.71 KB, patch)
2012-03-13 11:23 PDT, George Staikos
fpizlo: review-
ap: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Staikos 2012-03-12 19:13:25 PDT
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.
Comment 1 Alexey Proskuryakov 2012-03-13 11:04:58 PDT
CC'ing some JSC folks in case these warnings point to something more interesting than just a need to cast.
Comment 2 Alexey Proskuryakov 2012-03-13 11:05:36 PDT
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.
Comment 3 George Staikos 2012-03-13 11:16:07 PDT
Created attachment 131674 [details]
Don't define the static if it isn't used (ie: SA_RESTART defined)
Comment 4 George Staikos 2012-03-13 11:22:10 PDT
(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.
Comment 5 George Staikos 2012-03-13 11:23:51 PDT
Created attachment 131678 [details]
Fix signed/unsigned mismatches
Comment 6 Alexey Proskuryakov 2012-03-13 11:57:55 PDT
> 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.
Comment 7 Alexey Proskuryakov 2012-03-13 11:58:17 PDT
Comment on attachment 131678 [details]
Fix signed/unsigned mismatches

Still wrong casts.
Comment 8 George Staikos 2012-03-13 12:31:13 PDT
(In reply to comment #7)
> (From update of attachment 131678 [details])
> Still wrong casts.

Please enlighten me.
Comment 9 Alexey Proskuryakov 2012-03-13 12:43:38 PDT
> Please enlighten me.

Does comment 6 not help?
Comment 10 George Staikos 2012-03-13 13:07:42 PDT
(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.
Comment 11 Alexey Proskuryakov 2012-03-13 13:23:43 PDT
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.
Comment 12 George Staikos 2012-03-13 13:31:07 PDT
(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.
Comment 13 Filip Pizlo 2012-03-13 14:41:17 PDT
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)
Comment 14 Filip Pizlo 2012-03-13 14:42:11 PDT
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)
Comment 15 George Staikos 2012-03-13 14:52:04 PDT
(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.
Comment 16 WebKit Review Bot 2012-03-13 14:52:49 PDT
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>
Comment 17 WebKit Review Bot 2012-03-13 14:52:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Filip Pizlo 2012-03-13 14:58:08 PDT
(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.
Comment 19 Nikolas Zimmermann 2012-03-14 01:51:43 PDT
(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.
Comment 20 Adam Treat 2012-03-14 08:13:33 PDT
(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.
Comment 21 Alexey Proskuryakov 2012-03-14 09:21:49 PDT
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.
Comment 22 Adam Treat 2012-03-14 10:35:43 PDT
(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.