1265154 - Windows builds using MSVC 2013 fail with nsCSSProps.cpp(916) : error C2057: expected constant expression

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bill Gianopoulos [:WG9s]]

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

Comment 1

[Bill Gianopoulos [:WG9s]]

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");

Comment 2

[Bill Gianopoulos [:WG9s]]

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

[Nathan Froyd [:froydnj]]

Does the problem go away if you merge the two strings, rather than expecting the compiler to do it for you?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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

[Xidorn Quan [:xidorn] UTC+10]

(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

[Nathan Froyd [:froydnj]]

(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...

Comment 7

[Bill Gianopoulos [:WG9s]]

But to answer your question merging the 2 strings did not help at all.

Comment 8

[u459114]

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

Comment 9

[u459114]

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

Comment 10

[Bill Gianopoulos [:WG9s]]

Plan 2 looks best to me is simpler and just works everywhere.

Comment 11

[Bill Gianopoulos [:WG9s]]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please use MOZ_ARRAY_LENGTH() rather than using sizeof() manually.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(and a comment shouldn't be needed, as MOZ_ARRAY_LENGTH is widely used with static_assert)

Comment 14

[u459114]

Attachment: 0001-Bug-1265154-Fix-compile-error-in-MSVC-2013-caused-by.patch

Comment 15

[u459114]

Attachment: MozReview Request: Bug 1265154 - Fix compile error in MSVC 2013 caused by ArrayLength;

Review commit: https://reviewboard.mozilla.org/r/46973/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46973/

Comment 16

[u459114]

(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.

Comment 17

[Cameron McCormack (:heycam)]

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

Comment 18

[u459114]

Push into inbound already. (it looks like bot is PTO today.)

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06687f70665e

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06687f70665e