Closed
Bug 1265154
Opened 8 years ago
Closed 8 years ago
Windows builds using MSVC 2013 fail with nsCSSProps.cpp(916) : error C2057: expected constant expression
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: wgianopoulos, Assigned: u459114)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
An assert modified by the patch for bug 1265071 fails to compile under MSVC 2013. This change: static_assert(NS_STYLE_IMAGELAYER_CLIP_BORDER == NS_STYLE_IMAGELAYER_ORIGIN_BORDER && NS_STYLE_IMAGELAYER_CLIP_PADDING == NS_STYLE_IMAGELAYER_ORIGIN_PADDING && NS_STYLE_IMAGELAYER_CLIP_CONTENT == NS_STYLE_IMAGELAYER_ORIGIN_CONTENT, - "bg-clip and bg-origin style constants must agree"); + "Except background-clip:text, all {background,mask}-clip and " + "{background,mask}-origin style constants must agree"); causes this error: C:/ ... /layout/style/nsCSSProps.cpp(916) : error C2057: expected constant expression
Reporter | ||
Comment 1•8 years ago
|
||
Sorry. THIS is the assert that gets the error: static_assert(ArrayLength(nsCSSProps::kImageLayerOriginKTable) == ArrayLength(nsCSSProps::kBackgroundClipKTable) - 1, "background-clip has one extra value, which is text, compared" "to {background,mask}-origin");
Reporter | ||
Comment 2•8 years ago
|
||
And it is the patch for bug 759568 that caused the issue. Sometimes I have way too many bugs open at once in different tabs.
Comment 3•8 years ago
|
||
Does the problem go away if you merge the two strings, rather than expecting the compiler to do it for you?
Flags: needinfo?(wgianopoulos)
Comment 4•8 years ago
|
||
Could you try marking ArrayLength functions (in mfbt/ArrayUtils.h) constexpr directly rather than MOZ_CONSTEXPR and see whether it works? That seems to be something VS2013 should already support.
Comment 5•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #3) > Does the problem go away if you merge the two strings, rather than expecting > the compiler to do it for you? I don't believe this makes any sense...
Comment 6•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5) > (In reply to Nathan Froyd [:froydnj] from comment #3) > > Does the problem go away if you merge the two strings, rather than expecting > > the compiler to do it for you? > > I don't believe this makes any sense... Ah, good point; ArrayLength has been problematic in the past...
Updated•8 years ago
|
Flags: needinfo?(wgianopoulos)
Reporter | ||
Comment 7•8 years ago
|
||
But to answer your question merging the 2 strings did not help at all.
I am not that familiar with VS. Is it dangerous to do this change - https://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#55 # if _MSC_VER >= 1900 << Change to 1800(version of vs2013) # define MOZ_HAVE_CXX11_CONSTEXPR # define MOZ_HAVE_CXX11_CONSTEXPR_IN_TEMPLATES << does vs2013 support it? # define MOZ_HAVE_EXPLICIT_CONVERSION << does vs2013 support it? # endif
Plan 1. #ifdef MOZ_HAVE_CXX11_CONSTEXPR static_assert(ArrayLength(nsCSSProps::kImageLayerOriginKTable) == ArrayLength(nsCSSProps::kBackgroundClipKTable) - 1, "background-clip has one extra value, which is text, compared" "to {background,mask}-origin"); #else static_assert(sizeof(nsCSSProps::kImageLayerOriginKTable) / sizeof(KTableEntry) == sizeof(nsCSSProps::kBackgroundClipKTable) / sizeof(KTableEntry) - 1, "background-clip has one extra value, which is text, compared" "to {background,mask}-origin"); #endif Plan 2. Just replace ArrayLength by sizeof, since we are 100% sure sizeof is a constexpr static_assert(sizeof(nsCSSProps::kImageLayerOriginKTable) / sizeof(KTableEntry) == sizeof(nsCSSProps::kBackgroundClipKTable) / sizeof(KTableEntry) - 1, "background-clip has one extra value, which is text, compared" "to {background,mask}-origin"); Plan 3. # if _MSC_VER >= 1800 # define MOZ_HAVE_CXX11_CONSTEXPR
Reporter | ||
Comment 10•8 years ago
|
||
Plan 2 looks best to me is simpler and just works everywhere.
Reporter | ||
Comment 11•8 years ago
|
||
OH perhaps add a comment about why this is necessary under MSVC 2013 so that it could maybe be found and cleaned up later when we no longer support that compiler.
Please use MOZ_ARRAY_LENGTH() rather than using sizeof() manually.
(and a comment shouldn't be needed, as MOZ_ARRAY_LENGTH is widely used with static_assert)
Updated•8 years ago
|
Summary: Windows builds using MSVC 2013 fail → Windows builds using MSVC 2013 fail with nsCSSProps.cpp(916) : error C2057: expected constant expression
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46973/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46973/
Attachment #8742175 -
Attachment is obsolete: true
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #12) > Please use MOZ_ARRAY_LENGTH() rather than using sizeof() manually. That's help, thank you.
Attachment #8742180 -
Flags: review?(cam)
Comment 17•8 years ago
|
||
Comment on attachment 8742180 [details] MozReview Request: Bug 1265154 - Fix compile error in MSVC 2013 caused by ArrayLength; https://reviewboard.mozilla.org/r/46973/#review43575
Attachment #8742180 -
Flags: review?(cam) → review+
Assignee | ||
Comment 18•8 years ago
|
||
Push into inbound already. (it looks like bot is PTO today.)
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06687f70665e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•