Closed
Bug 1193459
Opened 9 years ago
Closed 9 years ago
Fix JS_DEPENDENT_TEMPLATE_HINT for clang-cl
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: away, Assigned: away)
References
Details
Attachments
(1 file)
2.05 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
clang-cl defines both _MSC_VER and __clang__, and behaves like Clang for purposes of this syntax.
Assignee: nobody → dmajor
Attachment #8646527 -
Flags: review?(jimb)
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)
Comment 4•9 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•9 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)
Comment 10•9 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
Closed: 9 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.
Description
•