[PATCH] D124635: Frontend: Delete output streams before closing CompilerInstance outputs
Duncan P. N. Exon Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 28 18:19:39 PDT 2022
dexonsmith added a comment.
In D124635#3481346 <https://reviews.llvm.org/D124635#3481346>, @benlangmuir wrote:
> LGTM, although I made a suggestion about the spelling.
Thanks for the review!
>> This fixes theoretical bugs related to proxy streams, which may have
>> cleanups to run in their destructor. E.g., buffer_ostream supports
>> pwrite() by holding all content in a buffer until destruction time.
> The general point about streams with destructors is well taken, but it seems a bit broken that `buffer_ostream` cannot `flush`. I guess the layering it just weird in that we have buffering in the base class, but `buffer_ostream` wants to do something different. Could we afford to make `flush` virtual? Not something needed for this patch.
I don't think making `flush()` virtual would help; `flush()` means "I want my changes to show up somewhere", but it does not mean "I'm done / EOF". `buffer_ostream` cannot forward to the buffered stream until it is sure that there will never be a(nother) `pwrite()` call. Otherwise it can't service the `pwrite()`.
But yeah, it's awkward.
What might be nice is if `createOutputFile()` took an argument saying "I don't need pwrite"; could implement that by not using `buffer_ostream`, and instead using a simple forwarding proxy that had `report_fatal_error` in `pwrite_impl()`.
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:796
+ OS = nullptr;
> Nit: `OS.reset(nullptr);` makes it more obvious this is a `unique_ptr` and not a raw pointer. Same for the other cases as well.
I see your point; I'll update.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits