ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.2k stars 391 forks source link

etl::optional doesn't compile with deleted copy constructor #146

Open mrdimfox opened 5 years ago

mrdimfox commented 5 years ago

If I use etl::optional with class like this:

struct A {
    A(int some) : _some(some) {}
    A(const A&) = delete;
    A(A&&) = delete;
    A& operator=(const A&) = delete;

  private:
    int _some;
};

and it does not compile with this error:

[build] ../src/etl/include\etl/optional.h:146:48: error: call to deleted constructor of 'A'
[build]      ::new (storage.template get_address<T>()) T(value_);
...
[build] ... note: 'A' has been explicitly marked deleted here
[build]     A(const A&) = delete;
[build]     ^

But it works ok if I use std::optional.

Version of ETL is 14.29.3. Compiler is "arm-none-eabi-gcc.exe (GNU Tools for Arm Embedded Processors 8-2019-q3-update) 8.3.1 20190703 (release) [gcc-8-branch revision 273027]".

jwellbelove commented 5 years ago

Can you check to see if std::optional works if you disable the assignment operator?

mrdimfox commented 5 years ago

Yes, it works, but only if you don't use the assignment. Seems logical.

jwellbelove commented 5 years ago

I'll have to figure out how std::optional constructs a new object from an existing one without using the copy constructor.

swagggpickle commented 5 years ago

I am curious in your example you have the move constructor for class A deleted as well. Is this accurate in your actual testing example?

jwellbelove commented 5 years ago

The C++17 STL version uses an rvalue reference value constructor, whilst the ETL's is C++03 style const reference. I shall add a conditional compile flag to choose an rvalue reference constructor if >= C++11.

mrdimfox commented 5 years ago

@swagggpickle, i have something like this:

#include <optional>

struct A {
    A(int some) : _some(some) {}
    A(const A&) = delete;
    A(A&&) = delete;
    A& operator=(const A&) = delete;

  private:
    std::optional<int> _some;
};

struct B {
    B(int a_val) : a(a_val) {}
    B() : a(std::nullopt) {}

    std::optional<A> a;
};

int main() {
    B b_with_some(1);
    B b_without_some;

    return 0;
}

Here is godbolt: https://godbolt.org/z/-XvjYM

jwellbelove commented 5 years ago

I've had a look at the code, but I still haven't bveen able to figure out yet how std::optional can create a non-trivially constructable object that has a deleted copy constructor, move constructor and assignment operator!

annius commented 5 years ago

Any chance the code for the struct is optimized out because b_with_some and b_without_some are never used? I noticed -O1 as a compiler option on godbolt.

mrdimfox commented 5 years ago

@annius Nope. You can remove it, the flag, and see the same picture: it is compiled successfully.

annius commented 4 years ago

@mrdimfox I mentioned this thread to a colleague and maybe he'll chime in himself, but he pointed out that if you look at the generated code on godbolt, there is nothing actually generated for any of the struct methods except for the constructor (even without optimization).

If your code is modified to actually use one of the deleted constructors (instead of simply instantiating the object), it will fail to compile.

For example, if you change the last-but-one line to

B b_without_some = without_some;
jwellbelove commented 4 years ago

If you read the documentation for std::optional, I think that the issue may be down to the conditional explicitness of the various std::optional constructors.

mrdimfox commented 4 years ago

@annius You are right, but problem is here: if I try to compile my code above with etl instead of std it does not compile. If I use std it does. I don't want to use a copy constructor explicitly like you did in the example. I want exact my code from the snippet above to be compiled.

Compiles:

#include <optional>

struct A {
    A(int some) : _some(some) {}
    A(const A&) = delete;
    A(A&&) = delete;
    A& operator=(const A&) = delete;

  private:
    std::optional<int> _some;
};

struct B {
    B(int a_val) : a(a_val) {}
    B() : a(std::nullopt) {}

    std::optional<A> a;
};

int main() {
    B b_with_some(1);
    B b_without_some;

    return 0;
}

Does not compile:

#include <etl/optional.h>

struct A {
    A(int some) : _some(some) {}
    A(const A&) = delete;
    A(A&&) = delete;
    A& operator=(const A&) = delete;

  private:
    etl::optional<int> _some;
};

struct B {
    B(int a_val) : a(a_val) {}
    B() : a(etl::nullopt) {}

    etl::optional<A> a;
};

int main() {
    B b_with_some(1);
    B b_without_some;

    return 0;
}
jwellbelove commented 4 years ago

The odd thing is that the compiler is trying to call A(const A&) when compiling B(int a_val) : a(a_val) {} I don't see why it should be calling the copy constructor for A when constructing with an int.

jwellbelove commented 3 years ago

It's a long time since I looked at this, but I now think that the etl::optional copy & move constructors need to have conditional overloads that memcpy values that are trivially copy constructible. If I make the changes then this will only work correctly if either the compiler supports the common traits built-ins or the user defines the overloaded traits for each type.