search

BurntSushi / toml

TOML parser for Golang with reflection.
MIT License
4.58k stars 528 forks source link

Fix metadata - do not reuse result of append #418

Open maroux opened 3 months ago

maroux commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

This change seems good, but you can't just add a new test in internal/toml-test. These tests are from https://github.com/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.