Skip to content

break: Handle codes not in the dictionary#230

Open
robert3005 wants to merge 4 commits into
developfrom
rk/handleoutofboundscodes
Open

break: Handle codes not in the dictionary#230
robert3005 wants to merge 4 commits into
developfrom
rk/handleoutofboundscodes

Conversation

@robert3005

@robert3005 robert3005 commented Jun 23, 2026

Copy link
Copy Markdown
Member

In order to avoid performance penalty of checked accesses we always pad the symbols table to the max length. This will require a change in consumers to avoid serialising padded values

fixes #218

Signed-off-by: Robert Kruszewski <github@robertk.io>
@codspeed-hq

codspeed-hq Bot commented Jun 23, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 30 untouched benchmarks


Comparing rk/handleoutofboundscodes (302e7cb) with develop (57969d6)

Open in CodSpeed

@robert3005 robert3005 requested review from AdamGS and a10y June 24, 2026 00:40

@AdamGS AdamGS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like a reasonable fix, if there's a way to make this faster we can do it as a followup.

Signed-off-by: Robert Kruszewski <github@robertk.io>
@robert3005 robert3005 changed the title Handle codes not in the dictionary break: Handle codes not in the dictionary Jun 24, 2026
Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Robert Kruszewski <github@robertk.io>

a10y commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

If I try and decode with a symbol table and I encounter a code not in that table I should panic shouldn't I? But instead this will just proceed?

@robert3005

Copy link
Copy Markdown
Member Author

not sure we should panic, we could offer a checked decompress that errors for symbols out of bounds?

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.

Potential soundness issue in fsst-rs 0.5.10

3 participants