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

Tests: Do not assume empty vectors alias #158

Merged
merged 2 commits into from Oct 19, 2023

Conversation

jakobnissen
Copy link
Collaborator

Currently, TS uses Base.mightalias to throw an error if the two inputs to three- arg transcode alias.
Two problems here: Base.mightalias is internal, and the new memory PR to Julia changes aliasing of zero-length arrays.
The temporary fix for now is to remove the test that tests that zero-length arrays trips the Base.mightalias. Later, let's fix this properly by not relying on Base internals in TS tests.

This fixes TranscodingStreams' test errors on JuliaLang/julia#51319

Currently, TS uses Base.mightalias to throw an error if the two inputs to three-
arg transcode alias.
Two problems here: Base.mightalias is internal, and the new memory PR to Julia
changes aliasing of zero-length arrays.
The temporary fix for now is to remove the test that tests that zero-length
arrays trips the Base.mightalias. Later, let's fix this properly by not relying
on Base internals in TS tests.
@vtjnash
Copy link
Contributor

vtjnash commented Oct 17, 2023

It is correct to assume that mightalias is the right function to use here. It is public. But just incorrect to assume that zero-element arrays have any aliasing as there is no memory region in common between them.

@jakobnissen
Copy link
Collaborator Author

How is it public? It's unexported, and it's docstring is not part of the Julia documentation. Nonetheless, it's good to hear that it can be used in packages :)

test/codecnoop.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Contributor

vtjnash commented Oct 17, 2023

It is the public facing part of Base.dataids, which is documented as being intended for extension by custom arrays.

@nhz2
Copy link
Member

nhz2 commented Oct 18, 2023

Looks good to me.
Maybe there could be an additional check for input.data === output.data but that could be in a different PR, and I'm not sure it is needed.

@jakobnissen jakobnissen merged commit 35edb68 into JuliaIO:master Oct 19, 2023
18 checks passed
@jakobnissen
Copy link
Collaborator Author

I'm just going to merge this now since it's a bugfix, to maintain compatibility with Julia's master branch.

@jakobnissen jakobnissen deleted the memory branch October 19, 2023 06:21
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.

None yet

3 participants