refactor: Return CAutoFile from BlockManager::Open*File() #28483
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
|
||
| // Disallow copies | ||
| BufferedFile(const BufferedFile&) = delete; | ||
| BufferedFile& operator=(const BufferedFile&) = delete; |
There was a problem hiding this comment.
Nit: Would removing this in the second commit when the FILE * is changed to CAutoFile make more sense?
There was a problem hiding this comment.
Now that fclose is removed, copies are allowed, because they no longer lead to a double-free undefined behavior segfault (Calling fclose twice on the same pointer)
This was only explicitly used in the tests, where it can be replaced by wrapping the original raw file pointer into a CAutoFile on creation and then calling CAutoFile::fclose(). Also, it was used in LoadExternalBlockFile(), where it can also be replaced by the (implicit call to the) CAutoFile destructor after wrapping the original raw file pointer in a CAutoFile.
This refactor allows to forward some calls to the underlying CAutoFile, instead of re-implementing the logic in the buffered file.
This is a refactor.
fa33b6f to
fa56c42
Compare
This is required for #28052, but makes sense on its own, because offloading logic to
CAutoFileinstead of re-implementing it allows to delete code and complexity.