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

Should we remove forwarding references from PTE solvers? #438

Open
Yurlungur opened this issue Nov 29, 2024 · 4 comments
Open

Should we remove forwarding references from PTE solvers? #438

Yurlungur opened this issue Nov 29, 2024 · 4 comments
Labels
bug Something isn't working clean-up Changes that don't affect code execution but make the code cleaner discussion help wanted Extra attention is needed interface Library interface work question Further information is requested

Comments

@Yurlungur
Copy link
Collaborator

The segfault on HIP was caused by an access-out-of-scope error caused by passing pointers by reference into the PTE solvers. See #437 . Perhaps we should consider removing the forwarding references and pass only by value into the PTE solvers (with the exception of PTE Solver Base, which requires pass-by-reference as it is used as the first part of the derived class constructors).

Ping @jhp-lanl @jdolence @dholladay00 @mauneyc-LANL and @rbberger for feedback

@Yurlungur Yurlungur added bug Something isn't working help wanted Extra attention is needed question Further information is requested discussion clean-up Changes that don't affect code execution but make the code cleaner interface Library interface work labels Nov 29, 2024
@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Dec 2, 2024

The problem was specifically with the accessors, yes? Those are designed to mimic pointers using the square bracket operator, yes?

@Yurlungur
Copy link
Collaborator Author

Yes but it's the difference between

RealAccessor_t &&rho

and

RealAccessor_t rho

where the former forwards the reference and the latter captures the accessor by value. For pointer-like types such as real pointers or views, these are equivalent since they have reference semantics.

But because the base class constructor captures by reference (and needs to) the forwarding reference (which is what we currently have) captures by reference too. That was the problem.

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Dec 2, 2024

But because the base class constructor captures by reference (and needs to) the forwarding reference (which is what we currently have) captures by reference too. That was the problem.

Ah okay, and they were capturing a reference to a temporary rather than the reference the temporary held, right?

And I suppose the only argument for keeping forwarding references here is that the accessors could potentially be arbitrarily complex and might be better to forward rather than copy, right? But in practice most accessors are probably just using a non-standard striding which only requires holding a couple ints, so the copy is cheap.

@Yurlungur
Copy link
Collaborator Author

Ah okay, and they were capturing a reference to a temporary rather than the reference the temporary held, right?

Yep exactly.

And I suppose the only argument for keeping forwarding references here is that the accessors could potentially be arbitrarily complex and might be better to forward rather than copy, right? But in practice most accessors are probably just using a non-standard striding which only requires holding a couple ints, so the copy is cheap.

Yeah. That and also forwarding references permits passing something that has deep copies by default like std::vector, whereas capturing by value insists on things with pointer/reference semantics that do shallow copy by default. But that's probably what we want anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working clean-up Changes that don't affect code execution but make the code cleaner discussion help wanted Extra attention is needed interface Library interface work question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants