Bug 235023

Summary: Fix C++20 build warnings with GCC
Product: WebKit Reporter: Lauro Moura <lmoura>
Component: Tools / TestsAssignee: Lauro Moura <lmoura>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, changseok, cmarcelo, esprehn+autocc, ews-watchlist, glenn, hi, joepeck, kangil.han, keith_miller, kondapallykalyan, luiz, mark.lam, msaboff, pangle, pdr, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 233963    
Bug Blocks: 195548    
Attachments:
Description Flags
Patch
none
Updated patch after rebase none

Description Lauro Moura 2022-01-09 21:26:00 PST
These warnings appeared after r287698 (the first stint of bug233963 in the tree before being reverted for small adjustments).

They're mostly about:

* `this` not being captured by default in [=] lambdas. (Largely the most common issue)
* Operations between enums of different types are deprecated.
Comment 1 Lauro Moura 2022-01-09 21:30:22 PST
Created attachment 448722 [details]
Patch
Comment 2 Lauro Moura 2022-01-09 21:31:59 PST
Oops. Thought that by adding the bug dependency it would pull the patch from the dependency.
Comment 3 Alex Christensen 2022-01-10 11:41:38 PST
Comment on attachment 448722 [details]
Patch

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

> Source/JavaScriptCore/assembler/X86Assembler.h:338
> +        return (TwoByteOpcodeID)(static_cast<int>(OP2_CMOVCC) + static_cast<int>(cond));

I think instead of adding two static_casts, we could just move the cast to static_cast<TwoByteOpcodeID>(cond)

> Source/WebCore/dom/ViewportArguments.h:77
> +    static constexpr int ValueAuto = -1;

Can we make this an enum class instead of just making them all an int with no type?

> Source/WebCore/dom/WheelEvent.h:35
> +    static const char TickMultiplier = 120;

constexpr
Also char isn't great for this.  Could it be a size_t?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:641
> +    static constexpr int MaxDimenstionForDirectCompositing = 2000;

float?
Comment 4 Lauro Moura 2022-01-14 21:47:53 PST
(In reply to Alex Christensen from comment #3)
> Comment on attachment 448722 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448722&action=review
> 
> > Source/JavaScriptCore/assembler/X86Assembler.h:338
> > +        return (TwoByteOpcodeID)(static_cast<int>(OP2_CMOVCC) + static_cast<int>(cond));
> 
> I think instead of adding two static_casts, we could just move the cast to
> static_cast<TwoByteOpcodeID>(cond)

Right, will do.

> 
> > Source/WebCore/dom/ViewportArguments.h:77
> > +    static constexpr int ValueAuto = -1;
> 
> Can we make this an enum class instead of just making them all an int with
> no type?

Wouldn't making it an enum class require static_casts to be used when doing operations with the raw types, even when float is their underlying type? (See https://godbolt.org/z/sqj88fM63 )

Btw, still regarding this enum, looks like `ValueLandscape` and `ValuePortrait` can be removed, as they aren't used anywhere since r256477 / 220631@main.

> 
> > Source/WebCore/dom/WheelEvent.h:35
> > +    static const char TickMultiplier = 120;
> 
> constexpr
> Also char isn't great for this.  Could it be a size_t?

It's used with an IntPoint in WheelEvent.cpp, so I've changed it to int to avoid "narrowing conversions" warnings.

> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:641
> > +    static constexpr int MaxDimenstionForDirectCompositing = 2000;
> 
> float?

Well spotted.
Comment 5 Lauro Moura 2022-01-14 22:35:04 PST
Created attachment 449243 [details]
Updated patch after rebase
Comment 6 EWS 2022-01-16 21:19:14 PST
Committed r288085 (246099@main): <https://commits.webkit.org/246099@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449243 [details].
Comment 7 Radar WebKit Bug Importer 2022-01-16 21:20:22 PST
<rdar://problem/87664817>