Bug 231980 - The code decoding std::optional<ImagePaintingOptions> can't be compiled by PlayStation due to the ImagePaintingOptions template constructor
Summary: The code decoding std::optional<ImagePaintingOptions> can't be compiled by Pl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Other Other
: P2 Normal
Assignee: ujwal koneru
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-19 13:51 PDT by ujwal koneru
Modified: 2021-10-24 13:44 PDT (History)
10 users (show)

See Also:

Attachments
Patch for templatized constructor compilation issues (1.86 KB, patch)
2021-10-19 13:51 PDT, ujwal koneru 		no flags Details | Formatted Diff | Diff
Patch for build fix (2.17 KB, patch)
2021-10-19 16:29 PDT, ujwal koneru 		no flags Details | Formatted Diff | Diff
WIP patch (757 bytes, patch)
2021-10-19 22:51 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch (2.02 KB, patch)
2021-10-20 13:09 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
compile error with -ftemplate-backtrace-limit=0 (47.51 KB, text/plain)
2021-10-21 18:29 PDT, Fujii Hironori 		no flags Details
Patch (2.22 KB, patch)
2021-10-21 22:31 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description ujwal koneru 2021-10-19 13:51:16 PDT 
Created attachment 441787 [details]
Patch for templatized constructor compilation issues

https://build.webkit.org/#/builders/65/builds/8136/steps/8/logs/errors

Error in compiling message with struct ImagePaintingOptions(https://github.com/WebKit/WebKit/blob/b78686d4f9686797562e0876902f30bf66b610cd/Source/WebCore/platform/graphics/ImagePaintingOptions.h)
Comment 1 ujwal koneru 2021-10-19 16:29:41 PDT 
Created attachment 441814 [details]
Patch for build fix

Fixed the style and changelog.
Comment 2 Ross Kirsling 2021-10-19 18:33:30 PDT 
This is a belated fix for r283941.
Comment 3 Ross Kirsling 2021-10-19 18:43:30 PDT 
Comment on attachment 441814 [details]
Patch for build fix

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

> Source/WebCore/ChangeLog:11
> +        https://bugs.webkit.org/show_bug.cgi?id=231980
> +
> +        Reviewed by NOBODY (OOPS!).

The explanation for the patch should come after these lines.
Comment 4 Fujii Hironori 2021-10-19 22:51:32 PDT 
Created attachment 441852 [details]
WIP patch

I don't know what's going on in the compiler, but the compiler can compile this patch. WDYT?
Comment 5 ujwal koneru 2021-10-20 09:22:55 PDT 
Fujii-san, your version looks better to me.

Just baffled why this would work and not the original code.
Comment 6 Fujii Hironori 2021-10-20 13:09:46 PDT 
Created attachment 441926 [details]
Patch
Comment 7 Ross Kirsling 2021-10-20 13:15:37 PDT 
Comment on attachment 441926 [details]
Patch

Nice!
Comment 8 Fujii Hironori 2021-10-20 14:07:35 PDT 
Comment on attachment 441926 [details]
Patch

Clearing flags on attachment: 441926

Committed r284566 (243308@main): <https://commits.webkit.org/243308@main>
Comment 9 Fujii Hironori 2021-10-20 14:07:39 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2021-10-20 14:08:21 PDT 
<rdar://problem/84478389>
Comment 11 Fujii Hironori 2021-10-21 13:36:33 PDT 
Still failing after the change. Reopned.
https://build.webkit.org/#/builders/42/builds/8221
Comment 12 Fujii Hironori 2021-10-21 18:29:04 PDT 
Created attachment 442101 [details]
compile error with -ftemplate-backtrace-limit=0
Comment 13 Fujii Hironori 2021-10-21 21:52:03 PDT 
I'd like to do something like the following, but didn't succeed.

> template <typename Type> static constexpr bool isOptionType = std::is_same_v<decltype(std::declval<ImagePaintingOptions>().setOption(std::declval<std::decay_t<Type>>())), void>;
> 
> template<typename First, typename... Rest, typename = std::enable_if_t<isOptionType<First>>>
> ImagePaintingOptions(First first, Rest... rest)

https://stackoverflow.com/q/59268030
Clang can't compile the code.
Comment 14 Fujii Hironori 2021-10-21 22:31:38 PDT 
Created attachment 442123 [details]
Patch
Comment 15 Ross Kirsling 2021-10-22 11:15:32 PDT 
Hmm, now it seems like Ujwal's original patch might be less complicated...
Comment 16 Fujii Hironori 2021-10-22 12:45:09 PDT 
(In reply to Ross Kirsling from comment #15)
> Hmm, now it seems like Ujwal's original patch might be less complicated...

It doesn't take multiple ImagePaintingOptions. Can it compile? I didn't try.
Comment 17 Fujii Hironori 2021-10-22 12:54:11 PDT 
(In reply to Fujii Hironori from comment #16)
> It doesn't take multiple ImagePaintingOptions. Can it compile? I didn't try.

Oh, EWS were green. attachment#441814 [details]
Comment 18 Darin Adler 2021-10-23 23:21:36 PDT 
Comment on attachment 442123 [details]
Patch

There must be a more elegant fix. But this is OK with me for now.
Comment 19 Fujii Hironori 2021-10-24 04:23:37 PDT 
Comment on attachment 442123 [details]
Patch

Thank you very much for the review. But, I have a new idea using explicit keyword now. I’ll try it befor landing this patch Monday.
Comment 20 Fujii Hironori 2021-10-24 13:39:35 PDT 
(In reply to Fujii Hironori from comment #19)
> I have a new idea using explicit keyword now.

There are a lot of implicit construction, and it doesn't solve the PlayStation issue.
I have no more ideas, I'm going to land comment#14 patch.
Comment 21 Fujii Hironori 2021-10-24 13:43:58 PDT 
Comment on attachment 442123 [details]
Patch

Clearing flags on attachment: 442123

Committed r284767 (243476@main): <https://commits.webkit.org/243476@main>
Comment 22 Fujii Hironori 2021-10-24 13:44:03 PDT 
All reviewed patches have been landed.  Closing bug.