oneTBB
oneTBB copied to clipboard
oneapi-src
Add missing definitions of constructors and assignment operators
This resolves gcc warnings about deprecated in C++11 implicitly generated assignment operators when user-defined constructors are present. This is because the constructors are inherited by a user-declaration from a base class, while there is no operator= inheriting in C++ and the using declaration for operator= does not prevent the compiler from implicitly generating the operators.
Since defining the operators then forces the compiler to implicitly define standard constructors as deleted, we also need to define the constructors as defaulted. We keep the using declaration for constructors to inherit the non-standard constructors.
Also, the commit replaces using-declarations of assignment operators with proper forwarding assignment operators. This fixes the returned type of the importer assignment operators (which is a reference to the base class, not the final class).
In concurrent_queue, the assignment operator was missing while copy/move constructors were present and contained non-trivial logic. The implicitly generated assignment operator would have been incorrect. The added assignment operator reuses copy/move constructors to implement assignment.
Fixes https://github.com/oneapi-src/oneTBB/issues/312. Fixes https://github.com/oneapi-src/oneTBB/issues/372. Fixes https://github.com/oneapi-src/oneTBB/issues/373.
PR has been rebased onto the current state of onetbb_2021 branch. It no longer adds constructors as they were added upstream.
I am OK with part @Lastique did. @anton-potapov could you please review tests? Notify: @kboyarinov
@Lastique , @isaevil This patch contains multiple logically independent parts. Some of them are pure bugs in the implementation, while other needs spec extensions/fixes and thus some more discussions. My suggestion is to split the patch into two parts - one with associative containers and second one with queues staff. This will allow us to speed up the integration of unquestionable parts.
@Lastique , @isaevil This patch contains multiple logically independent parts. Some of them are pure bugs in the implementation, while other needs spec extensions/fixes and thus some more discussions. My suggestion is to split the patch into two parts - one with associative containers and second one with queues staff. This will allow us to speed up the integration of unquestionable parts.
All code changes in this PR are fixing bugs, none are extensions or new features. I can split the changes into multiple commits, if that helps, but they are fixing the same class of bugs and as far as integration goes, all of them need to be merged in equal measure.
@Lastique, oneTBB has specification to conform with. For the associative containers it clearly specify return type of operator=. Current implementation of these containers does not conform with the spec and has to be fixed in any way.
As for the queues - their specification has no operator=, and thus to be changed first, before the implementation. This require time and discussions.
My point is to not block bug fixes in the implementation of associative containers with merge/approval of spec changes needed for queues.
The specification does not need to list the assignment operators in order for them to be present - they will be implicitly available if not deleted. IOW, as currently written, the spec says the assignment operators are (implicitly) present.
That is not a bug in the spec. The bug is that with current implementation of the queues, implicitly generated assignment is invalid. Again, this is purely an implementation issue, as a different implementation is possible where an implicitly generated assignment would have been correct.
You could argue that the spec should be explicit on the presence (or absence) of assignment operators, and that is fine and I would agree. However, regardless of the spec, the current implementation is incorrect and must be fixed.
In any case, I have separated the changes to the associative containers and queues into different commits. I did not touch the tests (as these were not authored by me), and I believe they cover both. So I'm not splitting this PR into two PRs. I don't think this is necessary anyway. You can feel free to cherry-pick individual commits, if you feel that some changes need to be merged while others do not.
@Lastique, as far as I see, copy/move assignment operators were added to concurrent queues in #1033. I am not sure about changes in the tests. Are they still applicable? If so, could you please rebase this PR to the current master? Otherwise, I will close it.
