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

Explicit definition of all ctrs, operator= and dstrs #515

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

Conversation

NikitaNikolaenko
Copy link
Contributor

To comply with the rule C.21 of C++ Core Guidelines:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all

This should have the same behavior as our implicitly-generated version.

Copy link
Contributor

@CodingCanuck CodingCanuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to remove myself from the reviewer list for this PR: the mechanics look good while skimming through the diffs, but IMHO this could use @benh 's eyes on what we intend to be copy/moveable vs. which constructors and assignment operators we want to delete.

@@ -18,6 +18,14 @@ class Field;
template <typename Value_>
class Field<Value_, false> {
public:
Field() = default;

Field(const Field&) = default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are some copy / move operations noexcept and others are not? I'd expect noexcept use to be consistent here: please document.

Glancing at code snippets below, I'd expect us to use noexcept everywhere we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have noexcept on copy operations (unless I made a typo?).

https://clang.llvm.org/extra/clang-tidy/checks/performance/noexcept-move-constructor.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have noexcept on copy operations (unless I made a typo?).

Why don't we have noexcept on our copy operations? Can our copy operations throw exceptions in a way that our move operation can't?

If not, it seems like our copy operations should also be noexcept.

@CodingCanuck
Copy link
Contributor

I'm going to remove myself from the reviewer list for this PR: the mechanics look good while skimming through the diffs, but IMHO this could use @benh 's eyes on what we intend to be copy/moveable vs. which constructors and assignment operators we want to delete.

Ooh, I don't know how to remove myself now that I've "left a review" :/ https://github.com/orgs/community/discussions/23054

If this is useful, here's a doc on how to dismiss PR reviews: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/dismissing-a-pull-request-review (this might be a note for myself if a re-review request comes up)

@NikitaNikolaenko
Copy link
Contributor Author

IMHO this could use @benh 's eyes on what we intend to be copy/moveable vs. which constructors and assignment operators we want to delete

This is one of the reasons why this PR was created :D
We have quite a lot of implicitness in our code right now (which I personally don't like) and this PR makes everyone think about whether we need this or that.

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

Successfully merging this pull request may close these issues.

2 participants