Opened 4 years ago

Closed 3 years ago

#2669 closed task (fixed)

Migration to C++11

Reported by: Yves Owned by:
Priority: Should Have Milestone: Alpha 18
Component: Core engine Keywords:
Cc: Patch:

Description (last modified by Yves)

For discussions and opinions, please refer to the forum thread here.

Overview

C++ is the next version of the C++ standard after C++98 and C++03. There's already C++14, but that's currently out of scope because it's definitely too early for that. This standard introduces many new features and addresses shortcomings of the language. C++11 still supports all features of the earlier C++ standards (with very few exceptions) and if the compiler supports it, it can be enabled for existing code by passing an argument to the compiler (no code changes required).

The C++11 standard consists of many new features that have to be supported by the compiler. Some of the features are more widely supported by older compilers and others would require newer compilers. In addition to saying yes or no to C++11, we also have to define which features we want to support and, as a consequence, which compiler versions we support.

As a reference, check Mozilla's C++11 guideline (draft).

For more information about C++11 refer to:

Decision

There's not really a decision if we want to switch to C++11 or not, it's more a decision if we want to switch now or later. As far as I know, C++11 will become mandatory at some point and it won't be possible anymore to switch it on or off. If we decide to switch now, we have to agree which subset of features we want to use and which ones not. Such a decision should probably be based on how well the features are supported by compilers and which versions of compilers we need to support.

Links for supported features

Some information on support for C++11 features in different compilers:

Pro/Contra of C++11

Pro

  • Improved performance (probably 1-3% when just enabling c++11 without additional changes. Depending on compiler and measuring conditions.). Additional improvements are possible by using the new language features correctly (like move constructors, improved template functionality, constexpr and others).
  • New standard was developed to improve shortcomings of C++ --> Provides better solutions in some cases (better error handling, improved maintainability, compile-time checks instead of runtime checks, style improvements etc.).
  • Allows us to use libraries that require C++11.

Contra

  • Requires programmers to understand the new C++ features
  • Requires additional care to avoid using features that are not supported as defined in our coding guidelines --> Could break compilation on other compilers
  • Although the new standard aims for improving the style of code, the mix of old code and new code and the additional possibilities could make it worse (hard to tell at this point)
  • Requires us to drop support for some versions of compilers

Required for

What we have to do for the migration

  • Create a new Autobuilder with C++11 support. The current one uses Visual Studio 2008 and will have to be replaced at some point anyway.
  • Specifying "C++11 supported features" table
  • Trivial change in premake4.lua
  • The ABI should be compatible in most cases and it should not be required to rebuild all Windows Binaries as far as I understand.

Attachments (1)

Clang_Patches.diff (1.4 KB) - added by Yves 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 4 years ago by Yves

Description: modified (diff)

comment:2 Changed 4 years ago by leper

wxWidgets 3 seems to require C++11 when building with clang++ (#2452), but I haven't tried reproducing it yet.

comment:3 Changed 4 years ago by wraitii

I agree wholeheartedly, new functions are nice, it's not that big of a change, and it'll set us forward. Furthermore C++11 seems very widely adopted.

comment:4 in reply to:  2 Changed 4 years ago by Ben Brian

Replying to leper:

wxWidgets 3 seems to require C++11 when building with clang++ (#2452), but I haven't tried reproducing it yet.

Looks like this bug: http://trac.wxwidgets.org/ticket/15915

They should try again with 3.0.1.

comment:5 Changed 4 years ago by Philip Taylor

About compiler support: Over the past 6 months, we had about 9 users on Ubuntu 11.10, 3K users on 12.04, 3K on 13.10, 3.5K on 14.04. Ubuntu 12.04 uses GCC 4.6. We don't need to support GCC on OS X any more (just use Clang instead). So I think it would be fine to require GCC 4.6, but not to require anything newer than 4.6 until 12.04 has dropped to insignificance (probably a couple of years).

On Windows, I see the following platforms reported for custom builds over the past 6 months:

OSCompilerUsers
Vista SP 2MSC 16002
Win7MSC 15001
Win7MSC 16001
Win7 SP 1MSC 15001
Win7 SP 1MSC 160017
Win7 SP 1MSC 17006
Win8MSC 16001
Win8MSC 17001
WinXP SP 3MSC 15002
WinXP SP 3MSC 16004
WindowsMSC 16006
WindowsMSC 17005

(1500 = VS2008, 1600 = VS2010, 1700 = VS2012)

So requiring VS2010 would be fine for most people. Requiring VS2012/2013 would force most people to upgrade, but hopefully nobody would mind much (since the Express editions are free). Except for the people on WinXP, which can't run VS2012+, so they'd have to upgrade Windows first (which is not free or easy).

comment:6 Changed 4 years ago by Ben Brian

Almost nobody is going to build SpiderMonkey on Windows though, so it would be fine if it needed a newer version of MSVC than the game itself (we need to find out what version it will require), although it would have to be built with XP compatibility as long as that's available and we care about it. That was an option in 2012, not sure about 2013 or newer.

Last edited 4 years ago by Ben Brian (previous) (diff)

comment:7 Changed 4 years ago by Ben Brian

Of course we don't support VS2013 yet anyway (#2271), but assuming that happens in the next release or two.

comment:8 Changed 4 years ago by Yves

I've tested my SpiderMonkey branch on Ubuntu 12.04 with GCC 4.6.3. Both SpiderMonkey and the game build fine.

comment:9 Changed 3 years ago by leper

Milestone: Alpha 17Alpha 18

SpiderMonkey 31 was moved, so move this too.

comment:10 Changed 3 years ago by Ben Brian

The game currently has some errors when building with C++11 and clang, on both OS X and FreeBSD, see #2804.

comment:11 Changed 3 years ago by Ben Brian

Should have referenced this ticket as well in r16157.

Here are a few more errors on OS X Yosemite building with C++11:

AtlasObjectImpl.cpp
In file included from ../../../source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp:18:
In file included from ../../../source/tools/atlas/AtlasObject/AtlasObject.h:32:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:439:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/algorithm:626:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/utility:294:15: error: no viable overloaded '='
        first = __p.first;
        ~~~~~ ^ ~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/map:610:15: note: in instantiation of member function 'std::__1::pair<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> >::operator=' requested here
        {__nc = __v.__cc; return *this;}
              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__tree:1263:35: note: in instantiation of member function 'std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> >::operator=' requested here
                __cache->__value_ = *__first;
                                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__tree:1204:9: note: in instantiation of function template specialization 'std::__1::__tree<std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> >, std::__1::__map_value_compare<const std::__1::basic_string<char>, std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> >, std::__1::less<const std::__1::basic_string<char> >, true>, std::__1::allocator<std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> > > >::__assign_multi<std::__1::__tree_const_iterator<std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> >, std::__1::__tree_node<std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> >, void *> *, long> >' requested here
        __assign_multi(__t.begin(), __t.end());
        ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/map:1617:21: note: in instantiation of member function 'std::__1::__tree<std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> >, std::__1::__map_value_compare<const std::__1::basic_string<char>, std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> >, std::__1::less<const std::__1::basic_string<char> >, true>, std::__1::allocator<std::__1::__value_type<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> > > >::operator=' requested here
            __tree_ = __m.__tree_;
                    ^
../../../source/tools/atlas/AtlasObject/AtlasObjectImpl.h:34:7: note: in instantiation of member function 'std::__1::multimap<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode>, std::__1::less<const std::__1::basic_string<char> >, std::__1::allocator<std::__1::pair<const std::__1::basic_string<char>, const AtSmartPtr<const AtNode> > > >::operator=' requested here
class AtNode
      ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:1330:19: note: candidate function not viable: 'this' argument has type 'const std::__1::basic_string<char>', but method is not marked const
    basic_string& operator=(const basic_string& __str);
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:1333:19: note: candidate function not viable: 'this' argument has type 'const std::__1::basic_string<char>', but method is not marked const
    basic_string& operator=(basic_string&& __str)
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:1337:45: note: candidate function not viable: 'this' argument has type 'const std::__1::basic_string<char>', but method is not marked const
    _LIBCPP_INLINE_VISIBILITY basic_string& operator=(const value_type* __s) {return assign(__s);}
                                            ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:1338:19: note: candidate function not viable: 'this' argument has type 'const std::__1::basic_string<char>', but method is not marked const
    basic_string& operator=(value_type __c);
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:1341:19: note: candidate function not viable: 'this' argument has type 'const std::__1::basic_string<char>', but method is not marked const
    basic_string& operator=(initializer_list<value_type> __il) {return assign(__il.begin(), __il.size());}
                  ^
In file included from ../../../source/tools/atlas/AtlasObject/AtlasObjectImpl.cpp:18:
In file included from ../../../source/tools/atlas/AtlasObject/AtlasObject.h:32:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:439:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/algorithm:626:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/utility:295:16: error: no viable overloaded '='
        second = __p.second;
        ~~~~~~ ^ ~~~~~~~~~~
../../../source/tools/atlas/AtlasObject/AtlasObject.h:64:17: note: candidate function not viable: 'this' argument has type 'const AtSmartPtr<const AtNode>', but method is not marked const
        AtSmartPtr<T>& operator=(const AtSmartPtr<T>& r) { if (&r != this) { dec_ref(); ptr = r.ptr; inc_ref(); } return *this; }
                       ^
../../../source/tools/atlas/AtlasObject/AtlasObject.h:63:17: note: candidate function not viable: 'this' argument has type 'const AtSmartPtr<const AtNode>', but method is not marked const
        AtSmartPtr<T>& operator=(T* p) { dec_ref(); ptr = p; inc_ref(); return *this; }
                       ^
2 errors generated.
make[1]: *** [obj/AtlasObject_Release/AtlasObjectImpl.o] Error 1
make: *** [AtlasObject] Error 2
Apple LLVM version 6.0 (clang-600.0.56) (based on LLVM 3.5svn)
Target: x86_64-apple-darwin14.0.0
Thread model: posix
Last edited 3 years ago by Ben Brian (previous) (diff)

comment:12 Changed 3 years ago by Yves

We have already figured out this one yesterday (const correctness problem when copying) and actually also one of those you fixed. Sorry, I should have told you about that. Anyway, I'll attach the patch here. I've not removed const in my solution but instead implemented a workaround for the clang bug that causes this error. I haven't committed it yet because the workaround needs C++11 and should be added when C++11 gets enabled.

Are there any issues left now? Is this here fixed when C++11 is enabled? http://pastebin.com/nzBqndbs

Last edited 3 years ago by Yves (previous) (diff)

Changed 3 years ago by Yves

Attachment: Clang_Patches.diff added

comment:13 Changed 3 years ago by Yves

In 16158:

Fix Atlas compile problems with C++11 and clang. Thanks to trompetin17 for investigating this.

Refs #2669

comment:14 in reply to:  12 ; Changed 3 years ago by Ben Brian

Replying to Yves:

We have already figured out this one yesterday (const correctness problem when copying) and actually also one of those you fixed. Sorry, I should have told you about that. Anyway, I'll attach the patch here. I've not removed const in my solution but instead implemented a workaround for the clang bug that causes this error. I haven't committed it yet because the workaround needs C++11 and should be added when C++11 gets enabled.

Is that really a clang bug though? I notice there hasn't been any update or progress on the ticket, and I was reading a bit on C++11 which made it seem (to me) that this could be explained by some interaction of the typecast operator constness and implicit copy/move operators. I also wasn't convinced that returning a const value made any sense there, so it semeed harmless to change.

Last edited 3 years ago by Ben Brian (previous) (diff)

comment:15 in reply to:  14 Changed 3 years ago by Yves

Replying to historic_bruno:

Is that really a clang bug though? I notice there hasn't been any update or progress on the ticket, and I was reading a bit on C++11 which made it seem (to me) that this could be explained by some interaction of the typecast operator constness and implicit copy/move operators. I also wasn't convinced that returning a const value made any sense there, so it semeed harmless to change.

I'm not an expert of the C++ standard, but for me it seems to be exactly what's described in that ticket, just with the copy assignment and not the copy constructor. Also if there are two compilers where it works one way and one compiler where it works the other way, someone must be wrong (or the standard fails because it doesn't describe the behaviour well enough).

I'm not sure if const is required/sensible there, though.

comment:16 Changed 3 years ago by Yves

In 16160:

Enable C++11 by default.

Don't generate workspaces and remove checks for unsupported compier/IDE version$

Refs #2669

comment:17 in reply to:  12 Changed 3 years ago by Ben Brian

Replying to Yves:

Are there any issues left now? Is this here fixed when C++11 is enabled? http://pastebin.com/nzBqndbs

The build works fine on Yosemite, but unfortunately I'm still getting that error on Lion w/ latest SVN.

comment:18 Changed 3 years ago by stanislas69

Every project Compiles on VS2013 Pro Windows 8.1.1 64Bits.

comment:19 Changed 3 years ago by Yves

In 16214:

SpiderMonkey 31 upgrade

This upgrade also introduces exact stack rooting (see to the wiki: JSRootingGuide) and fixes problems with moving GC. This allows us to enable generational garbage collection (GGC).
Measurements a few months ago have shown a performance improvement of a non-visual replay of around 13.5%. This probably varies quite a bit, but it should be somewhere between 5-20%. Memory usage has also been improved. Check the forum thread for details.

Thanks to everyone from the team who helped with this directly or indirectly (review, finding and fixing issues, the required C++11 upgrade, the new autobuilder etc.)! Also thanks to the SpiderMonkey developers who helped on the #jsapi channel or elsewhere!

Fixes #2462, #2415, #2428, #2684, #1374
Refs #2973, #2669

comment:20 Changed 3 years ago by Yves

In 16215:

Fix build with Visual Studio

I've reverted this workaround before the SM31 commit because I thought it's only a problem with VS2010. Actually VS2013 still doesn't support C++11 well enough and still requires the workaround.

Refs #2669, #2462

comment:21 Changed 3 years ago by leper

Resolution: fixed
Status: newclosed

Since we have CppSupport and everything else has already been done I'm closing this.

Note: See TracTickets for help on using tickets.