Skip to content

Avoid moving a return value, it might prevent (N)RVO#183

Merged
ivanpauno merged 2 commits intoign-math6from
ivanpauno/no-move-when-return
Dec 16, 2020
Merged

Avoid moving a return value, it might prevent (N)RVO#183
ivanpauno merged 2 commits intoign-math6from
ivanpauno/no-move-when-return

Conversation

@ivanpauno
Copy link
Copy Markdown
Contributor

@ivanpauno ivanpauno commented Dec 15, 2020

I'm building in Focal, this PR fixes some "-Wno-pessimizing-move" warnings (which does not happen with the gcc version in Bionic).

From here:

-Wno-pessimizing-move (C++ and Objective-C++ only)
This warning warns when a call to std::move prevents copy elision. A typical scenario when copy elision can occur is when returning in a function with a class return type, when the expression being returned is the name of a non-volatile automatic object, and is not a function parameter, and has the same type as the function return type.

struct T {
…
};
T fn()
{
  T t;
  …
  return std::move (t);
}
But in this example, the std::move call prevents copy elision.

This warning is enabled by -Wall.

@ivanpauno
Avoid moving a return value, it might prevent (N)RVO
fd5745e 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the bug Something isn't working label Dec 15, 2020
@ivanpauno ivanpauno self-assigned this Dec 15, 2020
@ivanpauno ivanpauno requested a review from scpeters as a code owner December 15, 2020 13:08
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Dec 15, 2020
@ivanpauno ivanpauno mentioned this pull request Dec 15, 2020
Merged
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 15, 2020 •
edited
Loading

Codecov Report

Merging #183 (22ffbad) into ign-math6 (e5f264b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #183   +/-   ##
==========================================
  Coverage      99.19%   99.19%           
==========================================
  Files             61       61           
  Lines           5982     5982           
==========================================
  Hits            5934     5934           
  Misses            48       48
Impacted Files Coverage Δ
include/ignition/math/graph/Graph.hh 100.00% <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 e5f264b...22ffbad. Read the comment docs.

chapulina
chapulina approved these changes Dec 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for catching that. We should enable more warnings on our Jenkins CI builds. Or maybe they'll already be enabled once we move CI to Focal.

@ivanpauno
Copy link
Copy Markdown
Contributor Author

ivanpauno commented Dec 15, 2020

We should enable more warnings on our Jenkins CI builds. Or maybe they'll already be enabled once we move CI to Focal.

It may be a combination of both things, the focal build already has some warnings in the logs that aren't being captured + current focal CI is using GCC 8 and I'm using the default Ubuntu Focal compiler (gcc 9.3.0).

The ignition_math-ci-pr_any-homebrew-amd64 failure seems unrelated to me.

@ivanpauno ivanpauno merged commit 0160cdf into ign-math6 Dec 16, 2020
@ivanpauno ivanpauno deleted the ivanpauno/no-move-when-return branch December 16, 2020 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scpeters scpeters Awaiting requested review from scpeters scpeters is a code owner

1 more reviewer

@chapulina chapulina chapulina approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ivanpauno ivanpauno

Labels

📜 blueprint Ignition Blueprint bug Something isn't working 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ivanpauno @chapulina @j-rivero @osrf-triage