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

Fix metadata - do not reuse result of append #418

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maroux
Copy link

@maroux maroux commented Jul 9, 2024

These lines added in #400 caused the problem since the result of multiple append calls can return two slices backed by the same backing array. See demo here: https://go.dev/play/p/2FGKuCvCUKz. The bug only surfaces under specific conditions when the capacity of parser.context is increased by more than 1 due to this append call.

Fixes #417

@arp242
Copy link
Collaborator

arp242 commented Jul 9, 2024

Last time I checked these:

a = make([]string, 0, N)
for i := 0; i < N; i++ {
        a = append(a, value)
}

a = make([]string, N)
for i := 0; i < N; i++ {
        a[i] = value
}

Have basically identical performance. And as a matter of style I've generally preferred using the append()-variant.

@arp242
Copy link
Collaborator

arp242 commented Jul 10, 2024

This change seems good, but you can't just add a new test in internal/toml-test. These tests are from https:/toml-lang/toml-test.

It's better to find an existing test that meets your requirements, or if there really aren't any existing tests yet I can add a new one to toml-test. Or alternatively.

I don't mind looking at this myself, but it might be a while as I'm rather busy at the moment.

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.

metadata.Keys (and thus metadata.Undecoded) is invalid in some cases
2 participants