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

Clarify that resize doesn't initialize memory #50560

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

noidexe
Copy link
Contributor

@noidexe noidexe commented Jul 17, 2021

Some users might expect resize() to initialize added elements to zero. This clarifies that it is not the case.

Only relevant to 3.x since 4.0 handles it differently

@noidexe noidexe requested a review from a team as a code owner July 17, 2021 17:48
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Assuming this is true, does it not apply to all the other Pool*Arrays?

doc/classes/PoolIntArray.xml Outdated Show resolved Hide resolved
@noidexe
Copy link
Contributor Author

noidexe commented Jul 17, 2021

Assuming this is true, does it not apply to all the other Pool*Arrays?

Not all but all the ones using primitives (see #43033). Added the note for Int, Byte and Real

@noidexe noidexe requested a review from YuriSizov July 17, 2021 21:48
@noidexe
Copy link
Contributor Author

noidexe commented Jul 17, 2021

#31040 also references the issue. This is fixed by #46476 on master by adding a fill(value) method but not yet backported to 3.x

@YuriSizov YuriSizov requested a review from a team September 3, 2021 20:21
@@ -134,6 +134,7 @@
</argument>
<description>
Sets the size of the array. If the array is grown, reserves elements at the end of the array. If the array is shrunk, truncates the array to the new size.
[b]Note:[/b] Added elements are not automatically initialized to 0 and may contain garbage.
Copy link
Member

Choose a reason for hiding this comment

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

"garbage" sounds colloquial and thus not really suitable for documentation. Or is that the official name of uninitialized memory?

Copy link
Member

@mhilbrunner mhilbrunner Oct 5, 2021

Choose a reason for hiding this comment

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

I mean, its not really colloquial, see e.g. "garbage collection", but in this case IMO "and may contain random values" or "previous values in memory" would be better.

@akien-mga
Copy link
Member

Commits should be squashed together, see PR workflow.

@YuriSizov YuriSizov dismissed their stale review October 7, 2021 20:51

Concerns were addressed, but I cannot validate them myself

@YuriSizov YuriSizov removed their request for review October 7, 2021 20:52
@akien-mga
Copy link
Member

For the record, I checked and confirmed that those poolarrays contains random data when resized:

	var pba = PoolByteArray()
	pba.resize(10)
	for i in pba:
		print(i)
	
	var pia = PoolIntArray()
	pia.resize(10)
	print(pia)
	
	var pra = PoolRealArray()
	pra.resize(10)
	print(pra)


It can often be all zeros for me (especially byte and float), but not always.

(For `PoolByteArray` apparently print just prints `[PoolByteArray]`...)

@noidexe
Copy link
Contributor Author

noidexe commented Oct 9, 2021

Getting all zeroes some times:
I'm not sure if the memory pool is initialized to zeroes when the game starts, but if it is it might be possible to be lucky and get resize to give you zeroes at first, but after a while you'll start getting garbage (i.e, memory that was used before).

"Garbage":
"Random values" is probably easier to understand than "garbage", "uninitialized memory", or any other technical term. The only problem is that it is not true. The values are not random. I'm worried someone might think it's a good way to get an array with random values.

@YuriSizov
Copy link
Contributor

Random invalid values, maybe?

@noidexe
Copy link
Contributor Author

noidexe commented Oct 9, 2021

Random invalid values, maybe?

They are valid, every arrangement of bits can be interpreted as a valid int or float, etc. And they are not random. "Garbage", or "garbage values" is the technical term.
resize() could initialize memory or not. This needs to be clarified.
Explaining what "garbage" is in the class reference would be like explaining what a variable is. I don't want to tell the user to RTFM but sometimes the terminology is unavoidable.

@timothyqiu
Copy link
Member

For "random value", I've seen many people without programming background think it means "the system will generate a random value there" and wonder why it decides to make the effort to generate a random value instead of just putting a zero 🤣

If "garbage value" is colloquial, I suggest "indeterminate value". It's used in C/C++ standards to describe such values.

@akien-mga
Copy link
Member

akien-mga commented Oct 9, 2021

Explaining what "garbage" is in the class reference would be like explaining what a variable is. I don't want to tell the user to RTFM but sometimes the terminology is unavoidable.

I don't agree that "garbage" and "variable" are at the same level terminology wise (for the record, PVS-Studio uses "garbage" in quotes, which they wouldn't do with variable or memory - quotes denote that it's not a naturally flowing term even in this context). I do think here it can be worth explaining what it is, because we have many beginner programmers who may not be familiar with the terms. And translators may also mistranslate it if it's not crystal clear in the English string. If all users knew about garbage in this context, you wouldn't even need to specify further that "resize does not initialize newly allocated memory".

I suggest "may contain garbage, i.e. indeterminate values".

Edit: BTW maybe it should not say "may", but "will". It's guaranteed to be uninitialized, thus indeterminate/garbage.

@noidexe
Copy link
Contributor Author

noidexe commented Oct 9, 2021

I suggest "maywill contain garbage, i.e. indeterminate values".

Thanks Akien and Timothyqiu. What I meant about variable is having to explain a term inside the class reference but "indeterminate value" is clear, accurate and short.
I'll commit the changes and make sure it complies with the PR workflow.

Some users might expect resize() to initialize added elements to zero. This clarifies that it is not the case.
@akien-mga akien-mga merged commit 90f8cd8 into godotengine:3.x Oct 18, 2021
@akien-mga
Copy link
Member

Thanks!

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.

5 participants