Clean up bencode implementation and tests#3916
Merged
Merged
Conversation
The idempotency spec calls `bencodable-obj-equal?', defined in `nrepl-tests-utils.el', without requiring it. The file only passed when some earlier test file in the suite happened to load that helper first. Running `eldev -p test -f test/nrepl-bencode-tests.el' on its own errored out with `void-function bencodable-obj-equal?'.
The docstring claims everything that isn't an integer/list/dict is encoded as a string, but the fallback branch was `(format "%s:%s" (string-bytes object) object)' -- and `string-bytes' errors on non-string input. So passing a symbol or float to `nrepl-bencode' crashed with `wrong-type-argument stringp ...'. Coerce to a string via `format' before measuring the byte length. The common path (string in, string out) is unchanged; previously-erroring inputs now encode as their printed form, matching the docstring.
Drop the intermediate sorted dict that was allocated only to be iterated immediately afterward. Sort the keys directly, walk them with `mapconcat', and skip the lambda wrapper around `string<' while there. For a dict with N keys the old code allocated 2N cons cells (via `nrepl-dict-put') only to throw them away. Same observable behavior, less garbage.
* Extract the invalid-bencode error path (log + ding + drain buffer) into `nrepl--bdecode-error'. The main decoder cond branch is now one line instead of eight, and the recovery semantics live with a docstring. * Document why `nrepl-bdecode' reuses a hidden buffer instead of `with-temp-buffer' -- it's a hot-path allocation choice, not the obvious idiom.
40+ lines of disabled benchmarks at the bottom of the test file -- last updated who knows when. If we want benchmarks again, they belong in a separate file under test/bench/ rather than rotting in the test suite.
Add specs that were missing: * Decoder: empty list (`le'), empty dict (`de'), zero integer (`i0e'), and strings whose contents happen to look like bencode (length-prefix means they're opaque data, not re-parsed). * Encoder: empty dict, zero integer, and the non-string fallback path (symbols and floats now coerced via `format' instead of crashing). * Round-trip property test across a variety of shapes. * Malformed-input handling -- the decoder must log + drain rather than crash, and `nrepl-bdecode' must be able to exit its loop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit of
nrepl-bencode.eland its test file turned up a few real bugs and some accumulated cruft. Each commit is independent and reviewable on its own.bencodable-obj-equal?without requiring the file that defines it. Runningnrepl-bencode-tests.elin isolation errored out; it only passed when something earlier in the suite happened to load the helper first.string-bytesdirectly, so passing a symbol or a float blew up withwrong-type-argument stringp .... Coerce viaformatfirst.nrepl--bencode-dict- dropped an intermediate sorted dict that was allocated only to be iterated immediately afterward. Sort keys andmapconcatdirectly.with-temp-buffer.