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.

Sign up for GitHub

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

Jump to bottom

Fix ExpectData compiler warnings #335

Merged
merged 3 commits into from
Mar 28, 2022
Merged

Fix ExpectData compiler warnings #335

merged 3 commits into from
Mar 28, 2022

Conversation

mjcarroll
Copy link
Contributor

Addresses #332

Signed-off-by: Michael Carroll michael@openrobotics.org

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Mar 15, 2022
@mjcarroll mjcarroll marked this pull request as draft March 15, 2022 20:13
@mjcarroll
Implement user assign operator to avoid implicit
ddf028b 
Addresses #332

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Just use =default
c84993d 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@codecov
Copy link

codecov bot commented Mar 15, 2022 •
edited
Loading

Codecov Report

Merging #335 (4fb8a69) into main (d140cfc) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 4fb8a69 differs from pull request most recent head 38c5cda. Consider uploading reports for the commit 38c5cda to get more accurate results

@@           Coverage Diff           @@
##             main     #335   +/-   ##
=======================================
  Coverage   75.65%   75.65%           
=======================================
  Files         128      128           
  Lines        5537     5538    +1     
=======================================
+ Hits         4189     4190    +1     
  Misses       1348     1348
Impacted Files Coverage Δ
include/ignition/physics/SpecifyData.hh 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d140cfc...38c5cda. Read the comment docs.

@mjcarroll mjcarroll marked this pull request as ready for review March 16, 2022 16:07
@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

azeey
azeey approved these changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

As I was reviewing this PR, I realized that the assumption that the base class' (CompositeData) copy ctor will be called only holds in certain scenarios. Here's a code sample that demonstrates when CompositeData copy ctor gets called: https://godbolt.org/z/6o6jWGvj8.

From that I see that having a default copy assignment operator would work as expected, but the way we have implemented ExpectData's copy constructor doesn't work properly if we tried to use it directly to copy ExpectData objects. It works fine when we use the derived class SpecifyData. I'll open a separate issue about this.

@azeey azeey mentioned this pull request Mar 28, 2022
Merged
6 tasks
@scpeters scpeters changed the title Remove deprecated copies Fix ExpectData compiler warnings Mar 28, 2022
@scpeters scpeters merged commit c36aee7 into main Mar 28, 2022
@scpeters scpeters deleted the remove_deprecated_copies branch March 28, 2022 18:00
azeey pushed a commit to azeey/gz-physics that referenced this pull request Mar 28, 2022
@mjcarroll @azeey
Fix ExpectData compiler warnings (gazebosim#335)
8cc2eef 
Use default assignment operator.
Addresses gazebosim#332.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
azeey pushed a commit to azeey/gz-physics that referenced this pull request Mar 30, 2022
@mjcarroll @azeey
Fix ExpectData compiler warnings (gazebosim#335)
3f28ba0 
Use default assignment operator.
Addresses gazebosim#332.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
scpeters pushed a commit that referenced this pull request Mar 30, 2022
@mjcarroll @scpeters
Fix ExpectData compiler warnings (#335)
7a52fdd 
Use default assignment operator.
Addresses #332.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll mentioned this pull request Apr 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azeey azeey azeey approved these changes

@scpeters scpeters scpeters approved these changes

@mxgrey mxgrey Awaiting requested review from mxgrey

Assignees
No one assigned
Labels
🌱 garden Ignition Garden
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mjcarroll @azeey @scpeters