Skip to content

Replace reference members with pointers #493

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

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

Conversation

glankk
Copy link
Contributor

@glankk glankk commented Aug 14, 2025

Having references as class members complicates copy/move constructors and assignment operators and in some cases prevents them from being defaulted. They are also a source of bugs, as there are currently several instances of copy/move operators assigning to their referred object where the intended behavior most likely is to rebind them. This PR replaces most reference members with pointers, and makes explicit some of the semantics of affected classes with an explicitly deleted/defaulted copy/move assignment constructor/operator.

Fixes clang-tidy issues in #421.

@danmar
Copy link
Owner

danmar commented Aug 14, 2025

I believe it is best practice to prefer reference instead of pointer;
https://isocpp.org/wiki/faq/references#refs-vs-ptrs

Having references as class members complicates copy/move constructors and assignment operators and in some cases prevents them from being defaulted.

Imho, not if the intention is that it should still reference the same global object. We want that "files" point at the same object.

They are also a source of bugs, as there are currently several instances of copy/move operators assigning to their referred object where the intended behavior most likely is to rebind them.

but just changing from reference to pointer does not solve that issue

@glankk
Copy link
Contributor Author

glankk commented Aug 14, 2025

I believe it is best practice to prefer reference instead of pointer; https://isocpp.org/wiki/faq/references#refs-vs-ptrs

I agree that references are preferable for public interfaces or trivial classes when possible, however semantically sensible copy and move assignments aren't really possible with non-trivial classes that contain references. References are still used in public-facing functions, only the internals are changed.

Imho, not if the intention is that it should still reference the same global object. We want that "files" point at the same object.

If we really want to have exactly one global files then I think the lesser evil would be to have a singleton.

but just changing from reference to pointer does not solve that issue

The bugs I'm referring to are on line 513, 529 and 1512 in simplecpp.cpp. It seems pretty clear to me that intent is to rebind the references, which is the behavior one would expect from an assignment operation, but that behavior is only possible with pointers. It's not needed for all classes though, Location is an example of a trivial class where a reference could be preferred.

@firewave
Copy link
Collaborator

Maybe this should be approached from the other end by resolving #424 first.

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