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

Avoid potential use of dangling temporary objects. #418

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

Conversation

scott-snyder
Copy link

In something like

  const auto& transformMatrix = detelement.nominal().worldTransformation();

nominal() returns a temporary Alignment object by value, and worldTransformation() returns a reference within that Alignment object. So we should make a copy.
(The C++ rules for extending the lifetime of a temporary bound to a reference don't help here because the object bound to a reference is different from the object whose lifetime needs to be extended.)

BEGINRELEASENOTES

  • Fix potential uses of dangling temporaries.
    ENDRELEASENOTES

In something like

```
  const auto& transformMatrix = detelement.nominal().worldTransformation();
```
nominal() returns a temporary Alignment object by value, and
worldTransformation() returns a reference within that Alignment object.
So we should make a copy.
(The C++ rules for extending the lifetime of a temporary bound to a reference
don't help here because the object bound to a reference is different
from the object whose lifetime needs to be extended.)
@tmadlener
Copy link
Contributor

Was this pointed out by a compiler flag / warning or did you get to know about this via some (I would assume non-trivial) debugging)? If the former, we should try to also get that into CI, IMHO.

@jmcarcell
Copy link
Member

Newer versions of clang report that, but this is only available in the LCG stacks I think

@tmadlener
Copy link
Contributor

k4geo should build on an LCG stack, right? It would be nice to avoid re-introducing this in the future.

@scott-snyder
Copy link
Author

Yeah, i saw compiler warnings. But i'm doing my own builds outside of the key4hep release, so compiler versions and to some extent compilation flags may be different.

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.

3 participants