eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.65k stars 385 forks source link

popo/user_trigger.hpp:33:5: error: exception specification of explicitly defaulted default constructor does not match the calculated one #494

Closed BH1SCW closed 3 years ago

BH1SCW commented 3 years ago

Required information

Operating system: Mac OS Mojave

Compiler version: Apple clang version 11.0.0 (clang-1100.0.33.12) Target: x86_64-apple-darwin18.6.0

Observed result or behaviour: iceoryx/iceoryx_posh/include/iceoryx_posh/popo/user_trigger.hpp:33:5: error: exception specification of explicitly defaulted default constructor does not match the calculated one UserTrigger() noexcept = default; ^ 1 error generated. gmake[2]: [posh/CMakeFiles/iceoryx_posh.dir/build.make:577: posh/CMakeFiles/iceoryx_posh.dir/source/popo/user_trigger.cpp.o] Error 1 gmake[1]: [CMakeFiles/Makefile2:384: posh/CMakeFiles/iceoryx_posh.dir/all] Error 2 gmake: *** [Makefile:172: all] Error 2

Expected result or behaviour: no build error

Conditions where it occurred / Performed steps: git commit id: commit f1c4b31c54cc7fccb352fc3cdda562d9a22fe638

elfenpiff commented 3 years ago

I did a first investigation and where unable to reproduce the bug on OS: Mac OS Big Sure Compiler: Apple clang version 12.0.0 (clang-1200.0.32.28)

@BH1SCW would it be possible for you to switch for now to the latest Mac OS Version with the latest clang compiler?

sculpordwarf commented 3 years ago

@elfenpiff I´ve seen this problem in Ubuntu 20.04 some time ago, too. Using the 'noexpect = default;' constructor seems to cause problems on different compilers. From my point of view, I would go for the compatible style in the code base. Maybe in some years it can be changed back. 'class MyClass { public: MyClass() noexcept {}}'

BH1SCW commented 3 years ago

I did a first investigation and where unable to reproduce the bug on OS: Mac OS Big Sure Compiler: Apple clang version 12.0.0 (clang-1200.0.32.28)

@BH1SCW would it be possible for you to switch for now to the latest Mac OS Version with the latest clang compiler?

Our company's vpn software has compatibility issues with the latest mac os, that‘s the reason why I still not upgrade my OS.

elBoberido commented 3 years ago

With the #490 PR this specific error should be fixed.

There are about 30 further usages of Ctor()/Dtor() noexcept = default;. The question is, shall we also implement the ctor/dtor for these or do we wait and do it on a case by case basis?

mossmaurice commented 3 years ago

The question is, shall we also implement the ctor/dtor for these or do we wait and do it on a case by case basis?

Are all those occurences leading to compile errors? I'm still struggeling to understand what causes the error. If a c'tor has noexcept in its signature, it can't be implicitly declared?

elBoberido commented 3 years ago

The question is, shall we also implement the ctor/dtor for these or do we wait and do it on a case by case basis?

Are all those occurences leading to compile errors? I'm still struggeling to understand what causes the error. If a c'tor has noexcept in its signature, it can't be implicitly declared?

From what I understand, some compiler have sometimes problems with it.

Maybe it's similar to defaulting a copy/move ctor. If there is a member which has no copy/move ctor, defaulting implicitly means deletion. In this case it might be the same with members without ctors with the noexcept specifier.

sculpordwarf commented 3 years ago

@mossmaurice At least it was a problem on my side with the 20.04 some time ago. I just changed the compiled stuff only, but for the master it needs to be everywhere corrected (else somebody builds e.g. a test and it fails again).

constructor does not match the calculated one
UserTrigger() noexcept = default;

If you remove the noexpect its gone or if you define the constructor empty.

elBoberido commented 3 years ago

Okay, I did a small test. If you have a member of a class with MemberCtor() noexcept(false) = default; the std::is_nothrow_default_constructible/std::is_nothrow_constructible traits evaluate to false for the class with that member, so my assumption was correct and the ctor gets deleted. I don't know it it makes sense to add this check to our tests, since it won't compile anyway if the ctor is deleted. This seems also highly compiler/STL version specific. There might be only two viable options, like @sculpordwarf already pointed out. With the removal of the noexcept specifier we could still create a test for std::is_nothrow_default_constructible to check if it's implicitly noexcept, but I have a slight leaning towards implementing an empty default constructor.

elfenpiff commented 3 years ago

...but I have a slight leaning towards implementing an empty default constructor.

I vote for implementing an empty default ctor!

elBoberido commented 3 years ago

...but I have a slight leaning towards implementing an empty default constructor.

I vote for implementing an empty default ctor!

on the other hand, this would also apply to defaulted copy/move ctors and we were not anymore able to do the rule of zero since a class without a ctor will implicitly be nothrowable except if it has a member which does throw, then it's implicitly throwable ... my head just banged on the table 🤕

mossmaurice commented 3 years ago

on the other hand, this would also apply to defaulted copy/move ctors and we were not anymore able to do the rule of zero since a class without a ctor will implicitly be nothrowable except if it has a member which does throw, then it's implicitly throwable ... my head just banged on the table face_with_head_bandage

Are you sure? Do you have a Godbolt snippet for that?

According to here and here, they even went back and forth in the committee :unamused:

If the user explicitly specifies an exception specification on a defaulted function, that’s the exception specification. Don’t delete the function, don’t reject the program, just accept it.

I suppose we should check if we can fix the clang build by changing the build to C++17 or adding some magic compiler option which works as written above.

elfenpiff commented 3 years ago

...but I have a slight leaning towards implementing an empty default constructor.

I vote for implementing an empty default ctor!

on the other hand, this would also apply to defaulted copy/move ctors and we were not anymore able to do the rule of zero since a class without a ctor will implicitly be nothrowable except if it has a member which does throw, then it's implicitly throwable ... my head just banged on the table

Just a short recap.

  1. We do not use or support exceptions anywhere (ensured through noexcept).
  2. A ctor() noexcept = default gets deleted if a member has a default ctor ctor() noexcept(false) which should be never the case in our code.

Therefore, we should use ctor() noexcept = default; and never implement it empty our selfs and when the problem arises we have an issue in a member which throws an exception in the ctor which is not allowed! Hence, not doing this and implementing it empty our self would hide the exception throwing ctor which would be a serious issue.

elBoberido commented 3 years ago

I don't have a godbolt snippet, but you can run this locally

#include <iostream>
#include <type_traits>

struct Bar {
    Bar() noexcept(false) = default;

    Bar(const Bar&) noexcept(false) = default;
    Bar(Bar&&) noexcept(false) = default;
};

struct Foo {
    Foo() noexcept = default;

    Foo(const Foo&) noexcept = default;
    Foo(Foo&&) noexcept = default;

    Bar b;
};
int main() {
    std::cout << "Is nothrow default constructible: " << std::is_nothrow_default_constructible<Foo>::value << std::endl;
    std::cout << "Is nothrow move constructible   : " << std::is_nothrow_move_constructible<Foo>::value << std::endl;
}

@elfenpiff I also came to the same conclusion, but what if the class is from the STL, e.g. atomic (I don't know if that's really the case, just as theoretical example)

elfenpiff commented 3 years ago

@elBoberido you are right but then we would have to write our own atomic since we explicitly do not allow any kind of exception. But I think the atomic does throw nothing so we should be safe in this case.

elBoberido commented 3 years ago

@elfenpiff the atomic was just an (maybe bad) example. The problem is that some types of the STL might cause the deletion of a defaulted noexcept ctor. That's out of reach for us to fix.

elBoberido commented 3 years ago

@elfenpiff I just found an example for an STL type which causes the deletion on a defaulted noexcept ctor when used as member

std::chrono::duration<int64_t, std::nano> d{0};
elfenpiff commented 3 years ago

@elfenpiff I just found an example for an STL type which causes the deletion on a defaulted noexcept ctor when used as member

std::chrono::duration<int64_t, std::nano> d{0};

And with this std::chrono as duration alternative is gone ...

elBoberido commented 3 years ago

@BH1SCW can you check if the current master works for you?

We need to find a proper solution to prevent this breakage in the future, so I would keep this issue open

BH1SCW commented 3 years ago

@BH1SCW can you check if the current master works for you?

We need to find a proper solution to prevent this breakage in the future, so I would keep this issue open

I just checked, current master is ok.

dkroenke commented 3 years ago

We need to find a proper solution to prevent this breakage in the future, so I would keep this issue open

MacOS Mojave (10.14) is available as image from GitHub CI.

Question is, do we need to support also the older versions of MacOS? I guess in future it can happen that the container is removed then. I would also vote against an additional build because the worker for MacOS are more limited than Ubuntu: https://docs.github.com/en/github/setting-up-and-managing-billing-and-payments-on-github/about-billing-for-github-actions.

One possibility is to install the older clang version on the actual MacOS runner and have an additional build step to compile with it.

elBoberido commented 3 years ago

@dkroenke I think we have to check what is needed to become Tier1 in ROS2 and align with that.

dkroenke commented 3 years ago

According to the current https://www.ros.org/reps/rep-2000.html for ROS2 Foxy and Galactic is MacOS Mojave (10.14) needed. I guess we can then tie the MacOS runners to that version. I can do it within this ticket here.

It can happen that in future MacOS support is downgraded to Tier3: https://github.com/ros-infrastructure/rep/pull/291/files But also there is Mojave mentioned.