clang-cl defines both _MSC_VER and __clang__, and behaves like Clang for purposes of this syntax.
Created attachment 8646527 [details] [diff] [review] patch
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?
Aha: the code moved in bug 1180017.
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+
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.
Third time's the charm :-/
Looks great. Thanks very much!
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.