[PATCH] D102736: Fix tmp files being left on Windows builds.

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 +

[PATCH] D102736: Fix tmp files being left on Windows builds.

Quuxplusone 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 +

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth added a comment. Nice catch Reid. 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 +

[PATCH] D102736: Fix tmp files being left on Windows builds.

rnk 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 +

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang added a comment. yep, I'll look into it, probably sometime tomorrow-- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://reviews.llvm.org/D102736 ___ cfe-commits mailing list cfe-c

[PATCH] D102736: Fix tmp files being left on Windows builds.

dyung added a comment. Hi Amy, after your change, the compile time for one of our internal test files went from about 1 second to 30 seconds. I have put a repro and the details in PR50628, can you please take a look? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang closed this revision. akhuang added a comment. Committed in 9d070b2f4889887f9ce497592ef01df7b9601a1c Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://re

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Yeah, this is good. My remaining comments are all speculations about how to improve this further in the future, but they aren't directly applicable to the goal here. Repository:

[PATCH] D102736: Fix tmp files being left on Windows builds.

rnk accepted this revision. rnk added a comment. I think this look good. Adrian, are your concerns addressed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://reviews.llvm.org/D102736 __

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang updated this revision to Diff 347802. akhuang marked 5 inline comments as done. akhuang added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://reviews.llvm.org/D102736 Files: clang/include/cla

[PATCH] D102736: Fix tmp files being left on Windows builds.

rnk added a comment. Nice, this seems to be working out. Comment at: clang/include/clang/Frontend/CompilerInstance.h:170 std::string TempFilename; +Optional TempFile; aganea wrote: > Can we always use `TempFile`? And remove `TempFilename` in that cas

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang added inline comments. Comment at: llvm/lib/Support/Path.cpp:1237 RenameEC = copy_file(TmpName, Name); setDeleteDisposition(H, true); } amccarth wrote: > I'm curious if this path has ever been exercised. > > I see that rename_handle is

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang updated this revision to Diff 347090. akhuang marked an inline comment as done. akhuang added a comment. Use TempFile on both linux and windows Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://reviews.llvm.org/D102736 File

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang marked 2 inline comments as done. akhuang added a comment. In D102736#2767432 , @aganea wrote: > Do you think the existing crash tests can be modified to validate that .tmp > files are deleted indeed? Looks like the existing crash tests use `#pr

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth added a comment. Cool! This is getting much simpler. My remaining comments are mostly musings and you can tell me to buzz off. But I'd like to see @aganea's questions addressed. It does seem redundant to have to keep the file name when we already have an object that represents the

[PATCH] D102736: Fix tmp files being left on Windows builds.

aganea added inline comments. Comment at: clang/include/clang/Frontend/CompilerInstance.h:170 std::string TempFilename; +Optional TempFile; Can we always use `TempFile`? And remove `TempFilename` in that case? Comment at: clang/lib/F

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang updated this revision to Diff 346882. akhuang added a comment. Change to using TempFiles Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://reviews.llvm.org/D102736 Files: clang/include/clang/Frontend/CompilerInstance.h

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang marked 2 inline comments as done. akhuang added a comment. In D102736#2769747 , @akhuang wrote: > In D102736#2769358 , @amccarth > wrote: > >> At some point, the duplicate handle must be closed. I don't

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang added a comment. In D102736#2769358 , @amccarth wrote: > At some point, the duplicate handle must be closed. I don't see that > happening. I've added an inline comment where I think it should be done. > > (I find it weird that duplicating the h

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth requested changes to this revision. amccarth added a comment. This revision now requires changes to proceed. Sorry, forgot to Request Changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://reviews.llvm.org/D102736

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth added a comment. At some point, the duplicate handle must be closed. I don't see that happening. I've added an inline comment where I think it should be done. (I find it weird that duplicating the handle seems necessary.) At a high level, it seems a shame that `llvm::support::fs` doe

[PATCH] D102736: Fix tmp files being left on Windows builds.

aganea added a comment. In D102736#2767516 , @amccarth wrote: > I seem to recall some thrashing on this topic a few months ago. If I'm > remembering correctly, setting the disposition to delete temporary files on > Windows was causing problems with Rus

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth added a comment. I'll have to dig into my archived email tomorrow. I seem to recall some thrashing on this topic a few months ago. If I'm remembering correctly, setting the disposition to delete temporary files on Windows was causing problems with Rust builds because you can't always

[PATCH] D102736: Fix tmp files being left on Windows builds.

rnk added a subscriber: jansvoboda11. rnk added a comment. For other reviewers, I know Duncan, @dexonsmith, has also spent some time working on the clang output file management APIs. And @jansvoboda11 as well, I think. Comment at: clang/lib/Frontend/CompilerInstance.cpp:720 +

[PATCH] D102736: Fix tmp files being left on Windows builds.

aganea added a comment. Do you think the existing crash tests can be modified to validate that .tmp files are deleted indeed? Comment at: clang/lib/Frontend/CompilerInstance.cpp:829 +Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text; +// Use OF_Delete on Win

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:872 // using stdin. + void *FileHandle = nullptr; +#ifdef _WIN32 I'm saving the windows HANDLE here instead of using `fd` because for some reason calling `_get_osfhandle(fd)`

[PATCH] D102736: Fix tmp files being left on Windows builds.

akhuang created this revision. akhuang added reviewers: rnk, aganea, amccarth. Herald added subscribers: dexonsmith, hiraditya. akhuang requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Clang writes object files by first wri