Fix JS_DEPENDENT_TEMPLATE_HINT for clang-cl

RESOLVED FIXED in Firefox 43

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

42 Branch
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
clang-cl defines both _MSC_VER and __clang__, and behaves like Clang for purposes of this syntax.
(Assignee)

Comment 1

3 years ago
Created attachment 8646527 [details] [diff] [review]
patch
Assignee: nobody → dmajor
Attachment #8646527 - Flags: review?(jimb)
(Assignee)

Comment 2

3 years ago
Hmm, so per one of the comments, I missed jsgc.h, but I don't find it here: https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=TEMPLATE_HINT and my local clang-cl builds didn't complain. Is jsgc.h a dead file or something?
Flags: needinfo?(jimb)
(Assignee)

Comment 3

3 years ago
Aha: the code moved in bug 1180017.
Flags: needinfo?(jimb)

Comment 4

3 years ago
Comment on attachment 8646527 [details] [diff] [review]
patch

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

Looks good to me, with the addition to the comments requested below.

::: js/public/TraceKind.h
@@ +97,5 @@
>  
>  // GCC and Clang require an explicit template declaration in front of the
>  // specialization of operator() because it is a dependent template. MSVC, on
>  // the other hand, gets very confused if we have a |template| token there.
> +#if defined(_MSC_VER) && !defined(__clang__)

You need to add to the comment here and in the other file, saying:

The clang-cl front end defines _MSC_VER, but still requires the explicit template declaration, so we must test for __clang__ here as well.

Otherwise, nobody will understand why that test is there.
Attachment #8646527 - Flags: review?(jimb) → review+

Comment 6

3 years ago
I asked you to comment *both* uses of this conditional. You only commented the first.

We really can't have an accumulation of bits and pieces that only one person (who has moved on to other things) understands the need for.

Please follow up. The patch can be DONTBUILD, since it doesn't affect any code.
Flags: needinfo?(dmajor)
(Assignee)

Comment 9

3 years ago
Third time's the charm :-/
Flags: needinfo?(dmajor)

Comment 10

3 years ago
Looks great. Thanks very much!
https://hg.mozilla.org/mozilla-central/rev/5703e8fcf382
https://hg.mozilla.org/mozilla-central/rev/4030c5ba9874
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.