Bug 187737

Summary: TestWTF.WTF_Expected.Unexpected is not MSVC-safe
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: achristensen, jfbastien, lforschler, stephan.szabo, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=188811

Description Ross Kirsling 2018-07-17 13:39:50 PDT 
The following test is compiler-dependent:

> constexpr const char* oops = "oops";
> ...
> constexpr auto s0 = makeUnexpected(oops);
> constexpr auto s1(s0);
> EXPECT_EQ(s0, s1);

It fails on Windows with the following error...

> ..\..\Tools\TestWebKitAPI\Tests\WTF\Expected.cpp:96
> Value of: s1
>   Actual: oops
> Expected: s0
> Which is: oops

...due to the fact that MSVC evidently keeps a separate string for each const char* variable when they're declared constexpr:
https://godbolt.org/g/HGC8nw
Comment 1 Ross Kirsling 2018-07-17 13:56:53 PDT 
(In reply to Ross Kirsling from comment #0)
> ...due to the fact that MSVC evidently keeps a separate string for each
> const char* variable when they're declared constexpr:
> https://godbolt.org/g/HGC8nw

Dunno why this link isn't working -- I made another one here:
https://godbolt.org/g/Bbwngz

As a solution, we could drop constexpr, add const, or use a value of a type other than const char*, but I want to make sure we're not losing some important coverage in doing so. JF, what would you prefer?
Comment 2 JF Bastien 2018-07-17 14:12:34 PDT 
(In reply to Ross Kirsling from comment #1)
> (In reply to Ross Kirsling from comment #0)
> > ...due to the fact that MSVC evidently keeps a separate string for each
> > const char* variable when they're declared constexpr:
> > https://godbolt.org/g/HGC8nw
> 
> Dunno why this link isn't working -- I made another one here:
> https://godbolt.org/g/Bbwngz
> 
> As a solution, we could drop constexpr, add const, or use a value of a type
> other than const char*, but I want to make sure we're not losing some
> important coverage in doing so. JF, what would you prefer?

This is a test of constexpr. Dropping constexpr defeats the purpose of the test. When I compile your second link with /O1 I get two identical functions. Seems like an MSVC bug? This is comparing the const char * pointers held in the unexpected object, they really should be the same.
Comment 3 Ross Kirsling 2018-07-17 15:29:50 PDT 
(In reply to JF Bastien from comment #2)
> This is a test of constexpr. Dropping constexpr defeats the purpose of the
> test. When I compile your second link with /O1 I get two identical
> functions. Seems like an MSVC bug? This is comparing the const char *
> pointers held in the unexpected object, they really should be the same.

I agree that it seems like an MSVC bug...

It seems allowable for the compiler to decide whether to keep one or two strings for the following:
> constexpr auto foo = "oops";
> constexpr auto bar = "oops";
(In fact, MSVC has a compiler option to control this: https://msdn.microsoft.com/en-us/library/s0s0asdt.aspx)
But to convert `constexpr auto bar(foo);` into `constexpr auto bar = "oops";` and thereby treat it as a new string seems questionable.

I certainly figured that constexpr was crucial to this test case, I just wondered whether changing the type or making it constexpr const (which evidently works) would be allowable. Either way, we can try submitting an MSVC issue.
Comment 5 Ross Kirsling 2018-08-22 15:28:59 PDT 
This is now resolved for Release mode as a side effect of bug 188811, as we were disabling string pooling for unclear reasons.

This will still fail in Debug though, so we can either leave this bug open to track the MSVC ticket linked above, or we could #if out the test for MSVC Debug with a FIXME comment.