Closed Bug 1300925 Opened 3 years ago Closed 3 years ago

MSVC compilation error in Intl.cpp with --without-intl-api after bug 1289934

Categories

(Core :: JavaScript Engine, defect)

35 Branch
x86_64
Windows 10
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: vlad, Assigned: vlad)

References

Details

Attachments

(1 file)

After bug 1289934 landed, building with MSVC (2015) and --without-intl-api results in compilation errors like the following:

Unified_cpp_js_src3.cpp
c:/proj/r/mozjs/mozjs/js/src/builtin/Intl.cpp(859): error C2971: 'ScopedICUObject': template parameter 'Delete': 'uenum_close': a variable with non-static storage duration cannot be used as a non-type argument
c:/proj/r/mozjs/mozjs/js/src/builtin/Intl.cpp(615): note: see declaration of 'ScopedICUObject'
c:/proj/r/mozjs/mozjs/js/src/builtin/Intl.cpp(128): note: see declaration of 'uenum_close'

In Intl.cpp, putting all the stub code in an anonymous namespace, and changing the functions to non-static fixes this issue, like in the attached patch.
Attachment #8788700 - Flags: review?(sphink)
Comment on attachment 8788700 [details] [diff] [review]
Use anon namespace for Intl stubs

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

Functions passed as non-type arguments to templates are required to have external or internal linkage, C++11 [temp.arg.nontype]p1.  And a name having namespace scope -- that is, one at top level or directly within a namespace -- has internal linkage if it's a function explicitly declared static, C++11 [basic.link]p3.  So this is an MSVC bug.

But this seems to basically work, I guess, and it doesn't really hurt anything else.  So okay.
Attachment #8788700 - Flags: review?(sphink) → review+
Vlad, looks like this is ready to land?

(I just ran into this issue myself, and was going to file a bug and then found this one, with an r+'d patch that's been sitting for 2 months. :))
Flags: needinfo?(vladimir)
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Vlad, looks like this is ready to land?
> 
> (I just ran into this issue myself, and was going to file a bug and then
> found this one, with an r+'d patch that's been sitting for 2 months. :))

Same here. I'll push the patch myself if I don't hear objections soon. :-)
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9822e0525b82
Change Intl.cpp stubs to an anonymous namespace. r=Waldo
https://hg.mozilla.org/mozilla-central/rev/9822e0525b82
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: needinfo?(vladimir)
You need to log in before you can comment on or make changes to this bug.