refactor: Use an enum for `Autofile::seek` wrapper #34077

pull rustaceanrob wants to merge 1 commits into bitcoin:master from rustaceanrob:12-15-afile changing 8 files +31 −20

rustaceanrob commented at 12:26 PM on December 15, 2025: contributor

The int origin argument of the seek method has no meaning to those without domain specific knowledge as to how FILE works. The only way for a caller to understand what this is doing is to enter the method body. This enum makes it clear as to what the origin argument represents at the expense of the Unix-specific options, which were unused anyway.
DrahtBot added the label Refactoring on Dec 15, 2025
DrahtBot commented at 12:26 PM on December 15, 2025: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34077.



Reviews

See the guideline for information on the review process.

Type Reviewers

Concept NACK sedited

Concept ACK l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
rustaceanrob force-pushed on Dec 15, 2025
DrahtBot added the label CI failed on Dec 15, 2025
DrahtBot commented at 12:45 PM on December 15, 2025: contributor


🚧 At least one of the CI tasks failed. <sub>Task Windows-cross to x86_64, ucrt: https://github.com/bitcoin/bitcoin/actions/runs/20232125325/job/58077489869</sub> <sub>LLM reason (✨ experimental): C++ compile error: invalid conversion from int to SeekFrom in streams_findbyte.cpp causing bench_bitcoin build failure.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
- Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
- A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
- An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.

</details>
rustaceanrob force-pushed on Dec 15, 2025
sedited commented at 2:25 PM on December 15, 2025: contributor

Tending towards Concept NACK as it is right now. I think it is fine to leak those types to the developer. The developer is already required to learn about FILE-specific error codes (e.g. for fclose), and internal behaviour of member functions such as truncate and commit. The docstring also states that it is just a wrapper around fseek.
l0rinc commented at 5:57 PM on December 15, 2025: contributor

I tend towards a Concept ACK, I don't like it when we're leaking abstractions. Saying that most of us are already familiar with obfuscated code isn't a good reason in my opinion to keep - especially since the enum kept the original names, just made the inputs bounded - which is a big win already.
in src/streams.cpp:1 in 7503083876 outdated
janb84 commented at 7:16 PM on December 15, 2025:

Is there a reason why lines 41

if (origin == SEEK_SET) {

and 43

} else if (origin == SEEK_CUR && m_position.has_value()) {

are not converted to the enum?

(test still pass after change)
rustaceanrob commented at 9:33 PM on December 15, 2025:

No reason not to, updated 5f26210db35e9460c62c8241091ed4a3f2590c23

Type	Reviewers
Concept NACK	sedited
Concept ACK	l0rinc

refactor: Use an enum for `Autofile::seek` wrapper

The `int origin` argument of the `seek` method has no meaning to those
without domain specific knowledge as to how `FILE` works. The only way
for a caller to understand what this is doing is to enter the method
body. This enum makes it clear as to what the `origin` argument
represents at the expense of the Unix-specific options, which were
unused anyway.

5f26210db3

rustaceanrob force-pushed on Dec 15, 2025
DrahtBot removed the label CI failed on Dec 15, 2025
rustaceanrob commented at 11:15 AM on December 29, 2025: contributor

Would there be use in a broader refactor of this type to use util::Result and specify an enum for fclose as well?
rustaceanrob commented at 2:04 PM on February 27, 2026: contributor

Closing due to lack of interest
rustaceanrob closed this on Feb 27, 2026

Contributors

Labels

refactor: Use an enum for `Autofile::seek` wrapper #34077

Code Coverage & Benchmarks

Reviews