Add a std::optional and std::variant implementation #5388

leoetlino · 2017-05-07T14:05:36Z

std::optional makes a few things a bit neater and less error prone. However, we still cannot use C++17 (unfortunately), so this commit adds an implementation of std::optional that we can use right now.

To do:

Fix building with compilers that have C++17 support (e.g. gcc 7.1). The c++17 flag needs to be enabled only when possible
Fix build failures on gcc 5 when using a unique_ptr with a std::optional. It looks like std::optional<std::unique_ptr> is considered to be copy constructible, when it really isn't...

sepalani · 2017-05-07T14:21:02Z

Should this file be modified or be compliant with our coding style? If so, should we keep these == true, == false?

Otherwise, it isn't used anywhere so I don't think it can harm, for the moment.

leoetlino · 2017-05-07T14:28:16Z

That's pretty easy to change, so there you go. Fixed.

Source/Core/Common/StdOptional.h

+  }
+
+  optional& operator=(optional&& rhs) noexcept(
+      is_nothrow_move_assignable<T>::value&& is_nothrow_move_constructible<T>::value)


Source/Core/Common/StdOptional.h

+
+  // 20.5.4.4, Swap
+  void swap(optional<T>& rhs) noexcept(
+      is_nothrow_move_constructible<T>::value&& noexcept(swap(declval<T&>(), declval<T&>())))


Orphis · 2017-05-08T08:08:21Z

Have you considered calling the file just "optional" and use __has_include_next and #include_next to use upstream's version instead?
It's supported by Clang and GCC.

leoetlino · 2017-05-08T08:47:47Z

I'm not really familiar with the process of "backporting" newer C++ stuff, so I just followed #351.

Is there any reason for using #include_next instead? (And what would its usage look like?) It doesn't seem to be standard or supported by MSVC.

Orphis · 2017-05-08T13:14:51Z

It's not standard anyway to provide anything in the std namespace yourself.
__has_include_next and #include_next are just ways to shadow headers with another one. You can check if there is another optional header available, and then use it. Your current file would be called optional too. That means the day we want to use the upstream version, we can just remove it.

As for MSVC, it does have it from 2017. We could up the requirement. And there might be a way to conditionally add the include path in vcxproj files.

Another way to do it is to do it is to test if the header exists from CMake. If it does, then we use it, or we add an include path that makes this version available.

Basically, I like compatibility headers to be transparent and with the same name :)

Orphis · 2017-05-08T13:16:24Z

Oh, all of that are suggestions that I think are worth exploring, not requirements in any way.

BhaaLseN · 2017-05-08T16:00:38Z

How else would you use soon-to-be-std functionality without putting your own stuff into std (sans a ton of #if standard use this #else ise that), if not by putting it into a std namespace (even when its not something you should be doing normally)?

We've done it before, it worked; but we mostly didn't have other alternatives available. I'm for exploring the alternatives and using standard headers if we can.
Not sure what the general consensus about VS2017 is though (which does have std::optional if i read the docs correctly)

lioncash · 2017-05-08T23:08:18Z

IMO, I think updating to VS2017 would be nice.

abhiaagarwal · 2017-05-13T15:55:44Z

Is this preferable to using STX Headers?

leoetlino · 2017-05-24T12:11:11Z

This is pretty much the implementation used in that repo.

Since the move to VS2017 doesn't seem to be happening soon (because of buildbot stuff), how should we proceed with this PR?

leoetlino · 2017-05-25T19:06:12Z

Okay, so we have now moved to VS2017, but I'm not sure about using #include_next. With clang, it is only supposed to be used in headers:

Note that __has_include_next, like the GNU extension #include_next directive, is intended for use in headers only, and will issue a warning if used in the top-level compilation file. A warning will also be issued if an absolute path is used in the file argument.

And to be honest, I don't see the benefit over a regular include. Whenever we want to use the upstream version, we would want to remove this implementation and replace the #include_next with an #include, which is not really much more different from changing the include from Common/StdOptional.h to optional.

Orphis · 2017-05-25T19:37:24Z

You're supposed to use the #include_next in the optional header itself, not in the code that uses it.
Another way, is to check in CMake if the header is supported by the compiler and use it. If it's not, then add the header to the include path.

BhaaLseN · 2017-05-26T07:47:48Z

VS 2017 supports std::optional, so do we still need this PR?

leoetlino · 2017-05-26T07:52:40Z

@Orphis Oh whoops, looks like I misread your suggestion. I'll change it to use the upstream version where possible with #include_next.

@BhaaLseN We do, if we don't want to require gcc 7 everywhere else :P

leoetlino · 2017-05-26T07:59:38Z

@Orphis It's been a while since I opened the PR, so I kind of forgot that we already use the upstream version where possible (__has_include(<optional>)). Is that good enough for you? __has_include is standard and is part of C++17.

Orphis · 2017-05-26T17:47:54Z

Well, I like compatibility changes to be non intrusive for the users.
So the header that people should include be <optional>.

So 2 options:

You have your header called optional always in the path and you check __has_include_next(<optional>) to use upstream version automatically when available, or fallback to your implementation.
You check from CMake if the compiler supports <optional>, if it doesn't you add your compatibility header to the include path.

Is that clear enough now? :)

leoetlino · 2017-05-26T19:37:48Z

@Orphis okay, I think I got it now. Would you mind reviewing this?

Source/VSProps/Base.props

@@ -80,6 +80,8 @@
      <RuntimeTypeInfo>false</RuntimeTypeInfo>
      <MinimalRebuild>false</MinimalRebuild>
      <MultiProcessorCompilation>true</MultiProcessorCompilation>
+      <!--Enable latest C++ standard-->
+      <LanguageStandard>stdcpplatest</LanguageStandard>


Orphis · 2017-05-26T20:42:49Z

Looks fine. Let's wait for #5484 first I think!

CMakeLists.txt

@@ -540,6 +540,13 @@ if(ANDROID)
  include_directories(Source/Android)
 endif()

+if(NOT CMAKE_CXX_COMPILER_ID MATCHES "MSVC")


Source/CMakeLists.txt

+else()
+  # Enable C++17, but fall back to C++14 if it isn't available.
+  # This would have been simpler if cmake supported CMAKE_CXX_STANDARD = 17;
+  # it currently doesn't, so this check has to be done manually.


std::optional makes a few things a bit neater and less error prone. However, we still cannot use C++17 (unfortunately), so this commit adds an implementation of std::optional that we can use right now. Based on https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/lib/gtl/optional.h which seems to be fairly similar to C++17's <optional> and standards compliant. It's one of the few implementations that handle propagating type traits like copy constructibility, just like libc++/libstdc++.

The whole NANDContentLoader stuff is truly awful and will be removed as soon as possible. For now, this fixes a bug that was exposed by std::optional::operator*.

Based on https://github.com/mpark/variant (which is based on libc++).

ligfx · 2017-06-03T22:44:55Z

This broke compiling DolphinQt2 with CMake, for the reasons outlined in https://gitlab.kitware.com/cmake/cmake/issues/16468

leoetlino force-pushed the optional branch from 3c2941c to 23c8bc9 Compare May 7, 2017 14:08

leoetlino force-pushed the optional branch from 23c8bc9 to ae96f59 Compare May 7, 2017 14:27

sepalani reviewed May 7, 2017

View reviewed changes

leoetlino force-pushed the optional branch from ae96f59 to 8e47dcf Compare May 7, 2017 15:06

leoetlino changed the title ~~[WIP] Common: Add a std::optional implementation~~ Common: Add a std::optional implementation May 7, 2017

leoetlino force-pushed the optional branch 4 times, most recently from 8c4bda0 to acc56d1 Compare May 26, 2017 17:31

leoetlino force-pushed the optional branch 2 times, most recently from 6db5a12 to 7c369a5 Compare May 26, 2017 19:35

Orphis reviewed May 26, 2017

View reviewed changes

leoetlino force-pushed the optional branch from 7c369a5 to 00f2d2c Compare May 26, 2017 21:34

leoetlino changed the title ~~Common: Add a std::optional implementation~~ Common: Add a std::optional and std::variant implementation May 26, 2017

leoetlino force-pushed the optional branch from 00f2d2c to 102ad41 Compare May 27, 2017 13:45

leoetlino mentioned this pull request May 27, 2017

Clean up the boot code #5492

Merged

leoetlino force-pushed the optional branch from 07097b6 to 8f8ef66 Compare May 27, 2017 19:08

BhaaLseN mentioned this pull request May 28, 2017

Windows: Enable MSVC Standards Conformance (/permissive-) #5484

Merged

leoetlino force-pushed the optional branch 4 times, most recently from eccb5eb to d733ef7 Compare May 30, 2017 09:02

leoetlino changed the title ~~Common: Add a std::optional and std::variant implementation~~ [WIP] Common: Add a std::optional and std::variant implementation May 30, 2017

leoetlino force-pushed the optional branch from d733ef7 to bea3b4d Compare June 2, 2017 19:17

leoetlino changed the title ~~[WIP] Common: Add a std::optional and std::variant implementation~~ Add a std::optional and std::variant implementation Jun 2, 2017

ligfx reviewed Jun 2, 2017

View reviewed changes

CMakeLists.txt Outdated

@@ -540,6 +540,13 @@ if(ANDROID)

include_directories(Source/Android)

endif()

if(NOT CMAKE_CXX_COMPILER_ID MATCHES "MSVC")

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ligfx reviewed Jun 2, 2017

View reviewed changes

Source/CMakeLists.txt Outdated

else()

# Enable C++17, but fall back to C++14 if it isn't available.

# This would have been simpler if cmake supported CMAKE_CXX_STANDARD = 17;

# it currently doesn't, so this check has to be done manually.

This comment was marked as off-topic.

Sign in to view

leoetlino force-pushed the optional branch 3 times, most recently from e33c4d1 to e254887 Compare June 3, 2017 10:32

leoetlino added 4 commits June 3, 2017 12:36

Use std::optional for ESFormats/SharedContentMap

545006f

DiscIO: Fix the wrong NAND root being used

62d08e2

The whole NANDContentLoader stuff is truly awful and will be removed as soon as possible. For now, this fixes a bug that was exposed by std::optional::operator*.

Common: Add a std::variant implementation

e38a66f

Based on https://github.com/mpark/variant (which is based on libc++).

leoetlino force-pushed the optional branch from e254887 to e38a66f Compare June 3, 2017 10:36

Orphis approved these changes Jun 3, 2017

View reviewed changes

leoetlino merged commit 4af514b into dolphin-emu:master Jun 3, 2017

leoetlino deleted the optional branch June 3, 2017 11:21

ligfx mentioned this pull request Jun 4, 2017

CMake: make Qt work when using a manual "-std=c++##" flag #5526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a std::optional and std::variant implementation #5388

Add a std::optional and std::variant implementation #5388

leoetlino commented May 7, 2017 •

edited

sepalani commented May 7, 2017

leoetlino commented May 7, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Orphis commented May 8, 2017

leoetlino commented May 8, 2017

Orphis commented May 8, 2017

Orphis commented May 8, 2017

BhaaLseN commented May 8, 2017

lioncash commented May 8, 2017

abhiaagarwal commented May 13, 2017 •

edited

leoetlino commented May 24, 2017

leoetlino commented May 25, 2017

Orphis commented May 25, 2017

BhaaLseN commented May 26, 2017

leoetlino commented May 26, 2017

leoetlino commented May 26, 2017

Orphis commented May 26, 2017

leoetlino commented May 26, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

Orphis commented May 26, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ligfx commented Jun 3, 2017

Add a std::optional and std::variant implementation #5388

Add a std::optional and std::variant implementation #5388

Conversation

leoetlino commented May 7, 2017 • edited

sepalani commented May 7, 2017

leoetlino commented May 7, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Orphis commented May 8, 2017

leoetlino commented May 8, 2017

Orphis commented May 8, 2017

Orphis commented May 8, 2017

BhaaLseN commented May 8, 2017

lioncash commented May 8, 2017

abhiaagarwal commented May 13, 2017 • edited

leoetlino commented May 24, 2017

leoetlino commented May 25, 2017

Orphis commented May 25, 2017

BhaaLseN commented May 26, 2017

leoetlino commented May 26, 2017

leoetlino commented May 26, 2017

Orphis commented May 26, 2017

leoetlino commented May 26, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

Orphis commented May 26, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ligfx commented Jun 3, 2017

leoetlino commented May 7, 2017 •

edited

abhiaagarwal commented May 13, 2017 •

edited