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)

x86_64
Windows
defect
Not set
blocker

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
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");
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.
Blocks: 759568
No longer blocks: 1265071
Does the problem go away if you merge the two strings, rather than expecting the compiler to do it for you?
Flags: needinfo?(wgianopoulos)
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.
(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...
(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...
Flags: needinfo?(wgianopoulos)
But to answer your question merging the 2 strings did not help at all.
Assignee: nobody → cku
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
Plan 2 looks best to me is simpler and just works everywhere.
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)
Summary: Windows builds using MSVC 2013 fail → Windows builds using MSVC 2013 fail with nsCSSProps.cpp(916) : error C2057: expected constant expression
Attachment #8742175 - Attachment is obsolete: true
(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 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+
Push into inbound already. (it looks like bot is PTO today.)
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.