Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing definitions of constructors and assignment operators #318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Lastique
Copy link
Contributor

@Lastique Lastique commented Dec 16, 2020

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 #312.
Fixes #372.
Fixes #373.

@Lastique
Copy link
Contributor Author

Lastique commented Apr 4, 2021

PR has been rebased onto the current state of onetbb_2021 branch. It no longer adds constructors as they were added upstream.

@isaevil
Copy link
Contributor

isaevil commented Nov 25, 2021

@Lastique I guess it needs to be merged into master, not release branch.

@Lastique Lastique changed the base branch from onetbb_2021 to master November 25, 2021 10:36
@Lastique Lastique force-pushed the fix_inheriting_members branch 2 times, most recently from 0370b6f to 9ea86a8 Compare November 25, 2021 17:27
@isaevil
Copy link
Contributor

isaevil commented Nov 29, 2021

I am OK with part @Lastique did. @anton-potapov could you please review tests?
Notify: @kboyarinov

@anton-potapov
Copy link
Contributor

@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
Copy link
Contributor Author

Lastique commented Dec 1, 2021

@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 Lastique force-pushed the fix_inheriting_members branch from 925cab5 to 69bce89 Compare December 1, 2021 15:13
@anton-potapov
Copy link
Contributor

@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.

@Lastique
Copy link
Contributor Author

Lastique commented Dec 1, 2021

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
Copy link
Contributor Author

Lastique commented Dec 3, 2021

Rebased on top of master.

@kboyarinov
Copy link
Contributor

@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.

@Lastique Lastique force-pushed the fix_inheriting_members branch from 52d125b to 2ec4626 Compare February 14, 2024 14:16
@Lastique
Copy link
Contributor Author

Rebased. The tests were added by @isaevil.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants