akrzemi1/Optional

"C'tor/d'tor is not implicitly called" error with Unreal Engine (VS 2017)

patrikhuber opened this issue · 6 comments

patrikhuber commented

Hi,

Unfortunately akrzemi1::optional doesn't compile in Unreal Engine projects. It gives the following errors:

Error	C4582	'akrzemi1::storage_t<T>::value_': constructor is not implicitly called	MyTestProject	optional.hpp	270	
Error	C4583	'akrzemi1::storage_t<T>::value_': destructor is not implicitly called	MyTestProject	optional.hpp	275

The lines in question are:

constexpr storage_t( trivial_init_t ) noexcept : dummy_() {};

and

  ~storage_t(){}

Outside of Unreal Engine projects, it of course works flawlessly. Unfortunately, Unreal Engine has its own build system (UBT), and it's very peculiar on how it uses Visual Studio. It's basically a VS 2017 project and uses the VS 2017 compiler, but in C++14 mode, with the compiler flags set by some Unreal Engine C# build scripts (yes, it's horrible...).

This issue arose in ThePhD/sol2 too: ThePhD/sol2#235. I think @ThePhD took the implementation of his optional from you, and modified it. He fixed it in this and this commit, with quite some extensive changes (which are beyond my C++ skills).

Now I would be interested in: Why does this occur (apparently) only when building akrzemi1::optional in Unreal Engine, which uses the VS 2017 compiler, so it should presumably work? What makes it fail? Could it be some peculiar compiler flag? Or is it a bug in akrzemi1::optional that @ThePhD fixed in his copy?

akrzemi1 commented

One thing that I remember from looking at Visual Studio last time is that it was not able to handle constexpr member functions. For this reason I gave up trying to implement std::optional on Visual Studio. Maybe this has changed. Maybe Visual Studion still does not correctly implement constexpr unions?

ThePhD commented

I did take my implementation of Optional from here. The license from this repo is still preserved in my docs and elsewhere.

I fixed this problem, but I don't remember how. I didn't make a pull request back here. I honestly don't expect to keep maintaining the optional for much longer: I'm writing a paper to the std committee to hopefully fix std::optional<T&>, in which case I won't be needing this external optional anymore.

As a side note, if you can, VS 2017 ships with an optional. You should use it, and you'll probably avoid these problems, no? (Unless you need optional references like I do, in which case: feel free to back-convert the sol2 implementation into here).

patrikhuber commented

Thank you very much both for your replies!
I think I wasn't 100% clear enough what's going on, apologies. So:

  • It works fine with the VS 2017 compiler in C++17 mode. But obviously it's not necessary there because it has std:: <optional>.
  • It works fine with the VS 2017 compiler in C++14 mode. This is the default mode and some people are restricted to this. There is no std:: <optional> available in this mode so akrzemi1::optional is a great substitute.
  • It does not work and produces above compiler errors with the VS 2017 compiler in an Unreal Engine solution, which uses the UnrealBuiltTool. They set the compiler to C++14 mode, so std:: <optional> is not available and a substitute is needed. It's not supported to change the mode to C++17 and also changing compiler flags (or even finding out what they're set to) is horrible thanks to the horrendeous UnrealEngine build system.

So I was hoping basically that the compiler error might help you guys to guess what could be going on.

akrzemi1 commented

If you look at the following links,

https://answers.unrealengine.com/questions/607946/anonymous-union-with-none-trivial-type.html
https://social.msdn.microsoft.com/Forums/en-US/c33e7d32-6e90-42ce-8ceb-2664f2cfa036/compiler-errors-c4582-and-c4583-constructordestructor-is-not-implicitly-called?forum=vcgeneral

The problem seems to be quite common. The former link suggests changing the configuration in UE4 build to not treat C4582 and C4583 as errors. I am not sure if it is doing the right thing though.

Changing the implementation from union storage to std::aligned_storage would solve your warning, but it would compromise the constexpr guarantee. And I cannot do this. Maybe you can afford using boost::optional, which provides std::aligned_storage implementation?

patrikhuber commented

Oh wow! Particularly the first link is awesome! I googled quite a lot about this but I never found that first link. That explains it really well. So it's a particular MSVC compiler warning that is only enabled under /Wall (which I think MSVC users normally don't use since it's a really crazy warning level, much higher than gcc's -wall), and Unreal Engine chose to enable exactly this warning and even further, treat it as error.

That certainly explains everything. Nice, thank you very much!

That basically also means that any users that uses your optional and uses MSVC, is affected by this warning - they just never see it because nobody enables this warning or treats it as errors. You probably have many MSVC users, and nobody seemed to have complained yet, so this warning can probably be ignored.

Thank you for your ideas regarding solutions - I will think about which one is best for my use case :-) None of them is great, but the Unreal Engine is horrible anyway from a C++ point of view, so it's a matter of finding the least horrible one.

russpowers commented

In case anyone runs into this issue in the future, you can suppress the warnings (I've had to do this kind of thing with many libraries due to UE4's annoying warnings-as-errors). Just copy optional.hpp from this repo into optional_original.hpp, then create optional.hpp in the same directory and put this inside:

#pragma once

#pragma warning(push)
#pragma warning(disable:4583)
#pragma warning(disable:4582)
#include "optional_original.hpp"
#pragma warning(pop)
  • 👍6