Closed Bug 877937 Opened 6 years ago Closed 6 years ago

build the jseng in c++11 mode when we do that for the rest of the tree

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(5 files)

it'd be nice to have c++11 stuff there too, and it seems to work locally.
Attachment #756300 - Flags: review?(ted)
Attachment #756300 - Flags: feedback?(jwalden+bmo)
Comment on attachment 756300 [details] [diff] [review]
bug 877937 - build the js engine in c++11 mode when we do that for the rest of the tree

This solves the problem of building SpiderMonkey as C++.  But if embedders don't use C++11, and our public headers start using C++11-only features without appropriate ifdefs/macros, we'll break stuff.  And I don't doubt that'll happen pretty quickly, if we let this disconnect occur.  So we should do whatever's necessary to add --std=gnu++0x into the compiler flags that embedders are supposed to use.  That would be done by augmenting the js-config.in script however's necessary, probably changing Makefile.in to do so.
Attachment #756300 - Flags: feedback?(jwalden+bmo)
Comment on attachment 756300 [details] [diff] [review]
bug 877937 - build the js engine in c++11 mode when we do that for the rest of the tree

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

I kind of wish this was all in a common m4 file instead of copy and pasted, but it'll do.
Attachment #756300 - Flags: review?(ted) → review+
Assignee: nobody → trev.saunders
> I kind of wish this was all in a common m4 file instead of copy and pasted,
> but it'll do.


that would force me to figure out more m4, and besides I need to change the jseng version around some so that we can make the js embedding stuff use -std= when appropriate.  However I think we can make some of this code go away fairly soon.
Comment on attachment 758907 [details] [diff] [review]
make js embeddors build as c++11 when we do

Waldo is this what you want?

I tested this on a machine with gcc 4.4 and -std=gnu++0x wasn't in the output of js-config --cflags, and on a machine with gcc 4.7 it was.
Attachment #758907 - Flags: feedback?(jwalden+bmo)
Attachment #758907 - Flags: review?(ted) → review+
I'd use an .m4 file like the one documented there

http://www.gnu.org/software/autoconf-archive/ax_cxx_compile_stdcxx_11.html
  
But that's just me.

Also why gnu++0x instead of gnu++11 or non gnu extension version -std=c++11?
(In reply to Hubert Figuiere [:hub] from comment #7)
> I'd use an .m4 file like the one documented there
> 
> http://www.gnu.org/software/autoconf-archive/ax_cxx_compile_stdcxx_11.html
>   
> But that's just me.

see comment 4 (this is just coppying stuff from the main configure until we can kill gcc not in c++11 mode which I think we can do in a couple weeks)

> Also why gnu++0x instead of gnu++11 or non gnu extension version -std=c++11?

its known to work with all the compilers we care about as evidenced by the rest of the tree, and we want the gnu extensions.
Attachment #758907 - Flags: feedback?(jwalden+bmo) → feedback+
I don't pretend to understand the name lookup rules that make it necessary to fully qualify the class name here, but not qualified is fine for method implementations, but it works so *shrug*
Attachment #761431 - Flags: review?(jwalden+bmo)
It seems clang is confused about how it should assign to this type when js::Swap() is called on it at jsgc.cpp:3082.  I don't see what we are getting from this copy constructor so lets just remove it and let the compiler generate it for us.
Attachment #761438 - Flags: review?(jwalden+bmo)
Comment on attachment 761431 [details] [diff] [review]
make clang on mac happy part 1

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

I think we ran into this somewhere else, recently, so this is totally fine.
Attachment #761431 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 761438 [details] [diff] [review]
make clang on mac happy part 2

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

This got added to attempt to fix an OS X bustage, but it didn't work, so I'm fine removing it.
Attachment #761438 - Flags: review?(jwalden+bmo) → review+
Please add a note to https://developer.mozilla.org/en-US/docs/SpiderMonkey/24 describing these changes, maybe linking to this bug for background.  Thanks!
Duplicate of this bug: 866123
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #769265 - Flags: review?(mh+mozilla) → review+
Note you need to copy the configure.in bits as well if you want that to work on standalone js builds, instead of relying from the fact that it's inherited from the top level configure.
https://hg.mozilla.org/mozilla-central/rev/c14a1283292e
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla24 → mozilla25
Duplicate of this bug: 862759
Depends on: 883381
Bug 883381 essentially reopens this bug. Yay for non-backout effective backouts.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 894240
Blocks: 894242
https://hg.mozilla.org/mozilla-central/rev/af7c55ced80b
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.