[PATCH] D102736: Fix tmp files being left on Windows builds.
Amy Huang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 9 16:14:54 PDT 2021
akhuang added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:857-858
+ OS.reset(new llvm::raw_fd_ostream(Temp->FD, /*shouldClose=*/false,
+ Binary ? llvm::sys::fs::OF_None
+ : llvm::sys::fs::OF_Text));
+ OSFile = std::string(TempPath.str());
----------------
Quuxplusone wrote:
> amccarth wrote:
> > rnk wrote:
> > > I think the bug is here: the third parameter is `bool unbuffered`, not file flags, so we are opening the file for unbuffered writing, and that is really slow.
> > Yowza! In addition to a fix, we need some memes.
> >
> > * Why do we even have that lever?
> > * This constructor has too many overloads. Please remove three.
> > * [Facepalm] `bool` parameters. [Double facepalm] Negative `bool` parameter names that default to `false`.
> > In addition to a fix, we need some memes.
>
> https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil/ ;)
> I count 8 different signatures for `raw_fd_ostream`'s constructor, 3 of which come from the offending default-argument'ed overload. And none of them are `explicit`, which means e.g. `{42, false}` is a valid argument for a function taking `const raw_fd_ostream&`.
>
> Unrelated to argument lists, but I would add "using raw `new` instead of `std::make_unique`" to this line's lengthy list of sins.
oh whoops, thanks for the catch - for now I guess I'll just remove the extra parameter
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102736/new/
https://reviews.llvm.org/D102736
More information about the llvm-commits
mailing list