Skip to content

docs: correct package usage and backend claims - #28

Draft
yperbasis wants to merge 1 commit into
masterfrom
review/readme-accuracy
Draft

docs: correct package usage and backend claims#28
yperbasis wants to merge 1 commit into
masterfrom
review/readme-accuracy

Conversation

@yperbasis

Copy link
Copy Markdown
Member

Summary

  • correct the module import path and the redeclared example variable
  • describe native and fallback dispatch without unconditional performance claims
  • replace machine-specific benchmark numbers with a reproducible command
  • explain why Sum(nil) is not a fair allocation comparison
  • add a runnable external-package example

Why

The current README imports a previous module path, does not compile when copied as one example, describes only part of the backend selection, and presents benchmark artifacts as library allocation behavior.

This is an independent alternative to #11. It deliberately avoids another static benchmark table because those numbers depend on the processor, OS, toolchain, and benchmark call shape.

This is a documentation correction, so TDD is not applicable. The new Go example compiles and checks both the one-shot and streaming output.

Verification

  • go test ./...
  • go test -race ./...
  • go test -tags purego ./...
  • go vet ./...
  • go mod tidy -diff
  • golangci-lint run (twice)

Comment thread README.md

### faster_keccak vs x/crypto/sha3

| Size | faster_keccak | x/crypto | Speedup |

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.

lost table

Comment thread README.md

```go
import "github.com/Giulio2002/faster_keccak"
import "github.com/erigontech/fastkeccak"

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.

The snippet imports the package without an alias but then calls keccak.Sum256 / keccak.Hasher. That compiles (the package clause is package keccak), but the import path's last element is fastkeccak, so a reader who copies this block and types what the path suggests gets undefined: fastkeccak. The PR's own example_test.go aliases the import; the README should match it.

Suggested change
import "github.com/erigontech/fastkeccak"
import keccak "github.com/erigontech/fastkeccak"

Comment thread README.md
Comment on lines +46 to +48
Compare benchmarks with the same one-shot or streaming call shape. Supplying
`nil` to `hash.Hash.Sum` asks it to allocate an output slice and should not be
used to infer whether the hash implementation itself allocates.

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.

The advice cannot be followed with the command above it. go test -bench . in this repo yields exactly the mismatched shapes this paragraph warns about: BenchmarkFasterKeccak is a one-shot Sum256(data), while BenchmarkXCrypto is Reset+Write+Sum(nil); on the streaming side BenchmarkFasterKeccakHasher ends with Sum256() (no alloc) but BenchmarkKeccakStreaming_Sha3 ends with Read(buf[:]) and only runs one size. So a reader who runs the suggested command still reads "x/crypto allocates 32 B/op" off Sum(nil) — the exact artifact this PR set out to stop presenting as library behavior.

Either adjust the benchmarks so a comparable pair exists (x/crypto one-shot, and a Sum(out[:0]) form), or name the pair that is comparable today.

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.

2 participants