Skip to content

fs: consistently use fsbridge:: for ifstream / ofstream #23857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
fanquake wants to merge 3 commits into from
Closed

fs: consistently use fsbridge:: for ifstream / ofstream #23857

fanquake wants to merge 3 commits into from

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Dec 24, 2021

Two of these changes are part of #20744, but are ok to do now, and reduce the diff that will eventually need to be reviewed in that PR. See commit messages for details.

@fanquake
fs: consistently use fsbridge for {i,o}fstream
2da5bc5 
Part of bitcoin#20744, but this can be done now, and will simplify the diff.
@fanquake
fs: add missing <cassert> include
5668ad4 
This is needed to prevent compilation failures once boost is removed,
however is still correct to include now, and reduces the diff in bitcoin#20744.

<string> is extracted from the defines because it is used for Windows
and non-Windows code, i.e get_filesystem_error_message().
@fanquake
fs: consistently use fsbridge for fopen()
bcb3676
@maflcko
Copy link
Member

maflcko commented Dec 24, 2021

Please clarify if this changes behavior in any way or if this is a refactor.

@hebasto
Copy link
Member

hebasto commented Dec 24, 2021

https://cirrus-ci.com/task/6421734977961984:

bench/bench.cpp: In function ‘void {anonymous}::GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>&, const string&, const char*)’:
bench/bench.cpp:30:37: error: use of deleted function ‘fs::path::path(std::string)’
   30 |     fsbridge::ofstream fout{filename};
      |                                     ^
In file included from ./test/util/setup_common.h:9,
                 from bench/bench.cpp:7:
./fs.h:50:5: note: declared here
   50 |     path(std::string) = delete;
      |     ^~~~
make[2]: *** [Makefile:13538: bench/bench_bitcoin-bench.o] Error 1

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 24, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #20744 (Use std::filesystem. Remove Boost Filesystem & System by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Dec 25, 2021
Merged
@fanquake
Copy link
Member Author

fanquake commented Jan 7, 2022

Closing as this is going in as part of #20744.

@fanquake fanquake closed this Jan 7, 2022
@fanquake fanquake deleted the always_use_fsbridge branch January 8, 2022 02:06
@maflcko
Copy link
Member

maflcko commented Jan 26, 2022

Not sure why this was closed?

It looks like the CI failure hinted at a bug that needs fixup? In all other places where strings from the command line are converted to paths, PathFromString is used, but not here in bench.

@fanquake
Copy link
Member Author

fanquake commented Jan 26, 2022

Not sure why this was closed?

It's part of #20744.

@maflcko
Copy link
Member

maflcko commented Jan 26, 2022

I think it might be worthwhile to fix the presumed bug in bench before 20744, in which case this should compile and can also be done before 20744?

@fanquake
Copy link
Member Author

fanquake commented Jan 26, 2022

Ok. I had deleted the branch, but can open a new PR.

@fanquake fanquake mentioned this pull request Jan 26, 2022
Merged
fanquake added a commit that referenced this pull request Jan 27, 2022
@fanquake
Merge #24167: fs: consistently use fsbridge:: for ifstream / ofstream
d87a37a 
5e8975e fs: consistently use fsbridge for fopen() (fanquake)
486261d fs: add missing <cassert> include (fanquake)
21f781a fs: consistently use fsbridge for {i,o}fstream (fanquake)

Pull request description:

  These changes are part of #20744, but are also ok to do now, and reduce the diff in that PR. See commit messages for details. Revived from #23857.

ACKs for top commit:
  laanwj:
    Code review ACK 5e8975e
  MarcoFalke:
    ACK 5e8975e 🏕

Tree-SHA512: ee2dc857ce2479b39b65615e689f934b962e580299b0e7a0c6361633402b0d61e6e4479f41f6480e2c46101264d93f330b8f7b57e56df95a7f77e046a4e44697
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022
@fanquake
Merge bitcoin#24167: fs: consistently use fsbridge:: for ifstream / o…
f9de29f 
…fstream

5e8975e fs: consistently use fsbridge for fopen() (fanquake)
486261d fs: add missing <cassert> include (fanquake)
21f781a fs: consistently use fsbridge for {i,o}fstream (fanquake)

Pull request description:

  These changes are part of bitcoin#20744, but are also ok to do now, and reduce the diff in that PR. See commit messages for details. Revived from bitcoin#23857.

ACKs for top commit:
  laanwj:
    Code review ACK 5e8975e
  MarcoFalke:
    ACK 5e8975e 🏕

Tree-SHA512: ee2dc857ce2479b39b65615e689f934b962e580299b0e7a0c6361633402b0d61e6e4479f41f6480e2c46101264d93f330b8f7b57e56df95a7f77e046a4e44697
@bitcoin bitcoin locked and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fanquake @maflcko @hebasto @DrahtBot