784309 - CheckedInt.h Intel C++ compilation issue

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Alex Kontos]

Attachment: 45742.png

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:14.0) Gecko/20120819 Firefox/14.0.1
Build ID: 20120819213618

Steps to reproduce:

Compiled Firefox 15 with Intel's C++ Compiler.


Actual results:

Compilation got stuck at dom_quickstubs.cpp, with CheckedInt.h producing errors such as typename is not allowed.
Screenshot attached.


Expected results:

Compilation should have proceeded as normal. Attempted to have dom_quickstubs.cpp compiled by cl and the rest by icl but to no avail.

Comment 1

[Alex Kontos]

I'm thinking this could be a bug with the compiler, but Firefox 14.0.1 compiled without issue. Where any changes made to SpiderMonkey?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

There definitely were changes in CheckedInt around this timeframe. This code compiles without warnings in GCC, Clang and MSVC. I don't understand the warning shown in your screenshot.

Comment 3

[Alex Kontos]

Attachment: Compilation Log

Comment 4

[Alex Kontos]

(In reply to Alex Kontos from comment #3)
> Created attachment 653874 [details]
> Compilation Log

I've attached the compilation log and used make. (As opposed to pymake in the screenshot).

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Rename IsSigned template paramter to IsTSigned to avoid conflict with struct name

Can you please try this patch? This seems to be a compiler bug, but a fair one as I am using the same identifier to mean two different things.

Comment 6

[Alex Kontos]

(In reply to Benoit Jacob [:bjacob] from comment #5)
> Created attachment 653882 [details] [diff] [review]
> Rename IsSigned template paramter to IsTSigned to avoid conflict with struct
> name
> 
> Can you please try this patch? This seems to be a compiler bug, but a fair
> one as I am using the same identifier to mean two different things.

The patch works. I'll report the bug to Intel.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

WORKSFORME isn't what you wanted here ;-)

We'll land that patch then. Thanks for reporting to Intel.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 653882 [details] [diff] [review]
Rename IsSigned template paramter to IsTSigned to avoid conflict with struct name

This patch is trivial enough and even improves the code as it was not great to use IsSigned for two different things.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 653882 [details] [diff] [review]
Rename IsSigned template paramter to IsTSigned to avoid conflict with struct name

Review of attachment 653882 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 10

[Jeff Walden [:Waldo]]

Any reason this hasn't landed?

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

I tend to forget to land patches. sorry about that.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c99a2f189c

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9c99a2f189c