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

Update use of Q_DISABLE_COPY. #1352

Merged
merged 2 commits into from Sep 25, 2021
Merged

Conversation

odysseus654
Copy link
Contributor

@odysseus654 odysseus654 commented Sep 19, 2021

This attempts to find and replace most instances of "do not copy this class" code with Q_DISABLE_COPY. This:

  • standardizes the code,
  • ensures that both the copy constructor and assignment operator are handled,
  • ensures use of the newer "= delete" syntax

There are a few places that I did not replace the code:

  • Two classes didn't obviously #include QObject (which defines Q_DISABLE_COPY) or didn't appear intended to, and
  • What the heck is happening with interface/src/avatar/MyHead.h, is this a typo or bug in the code?

Testing focus:

  • NONE - these changes should not have any effect outside of the compiler

@digisomni digisomni added allow-build-upload Allows the upload of a build for non white-listed users enhancement New feature or request needs CR (code review) labels Sep 19, 2021
@digisomni digisomni added this to In progress in 2022.1.0 Selene Release via automation Sep 19, 2021
@digisomni digisomni added this to the 2021.2.0 Selene Release milestone Sep 19, 2021
2022.1.0 Selene Release automation moved this from In progress to Review in progress Sep 19, 2021
@@ -23,8 +23,8 @@ class MyHead : public Head {

private:
// disallow copies of the Head, copy of owning Avatar is disallowed too
MyHead(const Head&);
MyHead& operator= (const MyHead&);
MyHead(const Head&) = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Q_DISABLE_COPY here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because Head != MyHead. Thus my question in the first comment as to whether this is a bug or typo in the code, I'm struggling to make sense of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, good point. I'll take a look at that later and see if I can figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to think about it... that is a non-copy constructor and thus deleting it has no effect, we're only focused on the compiler-generated objects which at this point are copy-constructor, move-constructor, assign-self and assign-move, right?

So at best that line is useless and is likely a typo. I'm okay with replacing it with a Q_DISABLE_COPY and see what breaks, if nothing does we can consider it good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think this is a mistake.

You can't copy a Head into a MyHead, because where would whatever is added in MyHead come from? Such a thing can't happen automatically, so forbidding it even if legal shouldn't do anything.

Also, the comment in Head.h is identical, suggesting that it was copy/pasted from there and left by mistake.

So I think just Q_DISABLE_COPY it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Think I already ended up doing that in a later commit too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I noticed the commit just a second too late.

libraries/shaders/src/shaders/Shaders.h Show resolved Hide resolved
@digisomni digisomni changed the title [low priority] code review: use of Q_DISABLE_COPY Code Review: use of Q_DISABLE_COPY. Sep 23, 2021
2022.1.0 Selene Release automation moved this from Review in progress to Reviewer approved Sep 25, 2021
@daleglass daleglass merged commit 891d555 into vircadia:master Sep 25, 2021
2022.1.0 Selene Release automation moved this from Reviewer approved to Done Sep 25, 2021
@digisomni digisomni added the CR Approved At least one code reviewer has approved the PR. label Sep 25, 2021
@odysseus654 odysseus654 deleted the pr/q_disable_copy branch September 25, 2021 21:19
@digisomni digisomni changed the title Code Review: use of Q_DISABLE_COPY. Housekeeping: Update use of Q_DISABLE_COPY. Oct 3, 2021
@digisomni digisomni added the housekeeping Code or documentation cleanup label Nov 28, 2021
@digisomni digisomni changed the title Housekeeping: Update use of Q_DISABLE_COPY. Update use of Q_DISABLE_COPY. Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-build-upload Allows the upload of a build for non white-listed users CR Approved At least one code reviewer has approved the PR. enhancement New feature or request housekeeping Code or documentation cleanup
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants