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

Implement Ref<T>::operator=(T*) #38232

Closed
wants to merge 1 commit into from

Conversation

lupoDharkael
Copy link
Contributor

@lupoDharkael lupoDharkael commented Apr 26, 2020

There are 4 ways to create a Ref:


Ref<T> myref;
my_ref.instance()

Ref<T> my_ref(memnew(T));

Ref<T> my_ref = memnew(T);

Ref<T> my_ref = Ref<T>(memnew(T));

All 4 are used in the source code but they do different things.
The first straight forward but requires 2 lines.
The 4th one is verbose and requires extra ref and unref.
The 3rd one is shorter than the 4th but is not as efficent as
we don't define operator=(T *from) so it tries to cast the pointer.

This PR creates that operator and changes the 4th from to th 3rd
in order to have a more compact Ref definition without unneeded
casting.

@lupoDharkael lupoDharkael requested review from bojidar-bg, vnen and a team as code owners April 26, 2020 15:27
There are 4 ways to create a Ref:

Ref<T> myref;
my_ref.instance()

Ref<T> my_ref(memnew(T));

Ref<T> my_ref = memnew(T);

Ref<T> my_ref = Ref<T>(memnew(T));

All 4 are used in the source code but they do different things.
The first straight forward but requires 2 lines.
The 4th one is verbose and requires extra ref and unref.
The 3rd one is shorter than the 4th but is not as efficent as
we don't define operator=(T *from) so it tries to cast the pointer.

This PR creates that operator and changes the 4th from to th 3rd
in order to have a more compact Ref definition without unneeded
casting.
@lupoDharkael
Copy link
Contributor Author

As additional info, there are around 260 occurrences of the 3rd type (the one this PR optimizes) not counting the additions presented here.
image

@Xrayez
Copy link
Contributor

Xrayez commented Apr 26, 2020

I think the source of these diverse usages come from both following the existing codebase and poor documentation: likely the References doc section needs an update too.

@sheepandshepherd
Copy link
Contributor

Are you sure the middle two forms don't call the existing conversion constructor for these initializations?

	Ref(T *p_reference)

That's what the documentation on converting constructors suggests should happen, instead of operator= being called.

The 4th form should also perform copy elision for initializations. They seem to fall under mandatory copy elision even. The less verbose forms are nicer though, for sure.

It might still be useful to have this operator, of course - are there are non-initialization T* assignments in the existing code that would call it? Should it call init_ref() like the converting constructor does? Newly-created References need to be initialized with that one rather than ref() so that refcount_init is set to 0.

@TechnoPorg
Copy link
Contributor

The operator part of this PR seems to have been superseded by #36189, but there are still cases of usage number 4 in the code.

@reduz
Copy link
Member

reduz commented Jul 28, 2022

I prefer not to have this, you currently dont need to do this often so its better for readibility and avoiding mistakes that stays as-is.

@YuriSizov
Copy link
Contributor

Closing as rejected (and severely outdated).

@YuriSizov YuriSizov closed this Feb 9, 2023
@YuriSizov YuriSizov removed this from the 4.0 milestone Feb 9, 2023
@lupoDharkael lupoDharkael deleted the ref-remove branch October 6, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants