Bug 211783 - -Wsign-compare warnings in FTLLowerDFGToB3.cpp and DFGSpeculativeJIT.cpp
Summary: -Wsign-compare warnings in FTLLowerDFGToB3.cpp and DFGSpeculativeJIT.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-12 09:22 PDT by Michael Catanzaro
Modified: 2020-05-12 14:04 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.75 KB, patch)
2020-05-12 09:25 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-05-12 09:22:20 PDT
This fixes -Wsign-compare warnings introduced in bug #206182. See https://bugs.webkit.org/show_bug.cgi?id=206182#c47.

Note that I'm only submitting a *simple* solution. JSValue::BigInt32Mask is currently a negative-valued signed integer, and I'm skeptical that that's desirable, since unsigned is usually better to represent bit patterns. I'm not volunteering to try changing that, though.
Comment 1 Michael Catanzaro 2020-05-12 09:25:35 PDT
Created attachment 399134 [details]
Patch
Comment 2 Mark Lam 2020-05-12 09:31:04 PDT
Comment on attachment 399134 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3600
> +        static_assert(JSValue::BigInt32Mask == static_cast<int64_t>(0xfffe000000000012));

Would using a "ll" suffix suffice?  0xfffe000000000012ll

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3346
> +            static_assert(JSValue::BigInt32Mask == static_cast<int64_t>(0xfffe000000000012));

Ditto.
Comment 3 Darin Adler 2020-05-12 10:36:57 PDT
Comment on attachment 399134 [details]
Patch

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

r=me because I don’t see a better solution

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3600
>> +        static_assert(JSValue::BigInt32Mask == static_cast<int64_t>(0xfffe000000000012));
> 
> Would using a "ll" suffix suffice?  0xfffe000000000012ll

I think that won’t work because the value doesn’t fit in a 64-bit long long, and so the language makes it unsigned long long in that case. I consulted https://en.cppreference.com/w/cpp/language/integer_literal to explore this.
Comment 4 EWS 2020-05-12 10:40:25 PDT
Committed r261564: <https://trac.webkit.org/changeset/261564>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399134 [details].
Comment 5 Radar WebKit Bug Importer 2020-05-12 10:41:15 PDT
<rdar://problem/63143618>
Comment 6 Michael Catanzaro 2020-05-12 14:04:15 PDT
(In reply to Darin Adler from comment #3)
> I think that won’t work because the value doesn’t fit in a 64-bit long long,
> and so the language makes it unsigned long long in that case. I consulted
> https://en.cppreference.com/w/cpp/language/integer_literal to explore this.

Exactly. I had consulted the same page to figure out how integer literals work. Learning is good!