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

1. 
   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.
2. DrahtBot added the label Refactoring on Dec 15, 2025
3. 
   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 <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

4. rustaceanrob force-pushed on Dec 15, 2025
5. DrahtBot added the label CI failed on Dec 15, 2025
6. 
   DrahtBot commented at 12:45 pm on December 15, 2025: contributor

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

   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.

7. rustaceanrob force-pushed on Dec 15, 2025
8. 
   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.
9. 
   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.
10. 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

    0if (origin == SEEK_SET) {
    

    and 43

    0} 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
11. 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
12. rustaceanrob force-pushed on Dec 15, 2025
13. DrahtBot removed the label CI failed on Dec 15, 2025
14. 
    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?

Contributors
rustaceanrob DrahtBot sedited l0rinc janb84

Labels
Refactoring