akhuang marked 2 inline comments as done. akhuang added a comment. In D102736#2767432 <https://reviews.llvm.org/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 `#pragma clang __debug crash` but in the case where clang crashes it's actually fine. I guess the only cases where this patch actually applies is when the program is killed -- don't know if there's a good way to do that in the test suite? ================ Comment at: clang/lib/Frontend/CompilerInstance.cpp:733 + } else { + if (std::error_code EC = + llvm::sys::fs::rename(OF.TempFilename, NewOutFile)) ---------------- aganea wrote: > Here you can probably use `ECError` or `errorCodeToError()` with what is said > slightly above. I think this path goes away if we always use TempFile ================ Comment at: clang/lib/Frontend/CompilerInstance.cpp:840 int fd; - std::error_code EC = llvm::sys::fs::createUniqueFile( - TempPath, fd, TempPath, - Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text); +#if _WIN32 + // On Windows, use llvm::sys::fs::TempFile to write the output file so ---------------- aganea wrote: > Usually it is better to avoid platform-specific logic. Could we use > `TempFile` all the time on all platforms? Hm, probably, that would make things simpler. ================ Comment at: llvm/lib/Support/Path.cpp:1253 } sys::DontRemoveFileOnSignal(TmpName); #endif ---------------- amccarth wrote: > Possibly stupid idea: What if RemoveFileOnSignal and DontRemoveFileOnSignal > did the Win32-specific delete disposition flag twiddling? With that > encapsulated, and assuming we can still explicitly delete a Windows file > that's already marked for deletion, it seems like all of the special handling > here could go away. > > Then again, the ...OnSignal things are in sys rather than sys::fs, so maybe > it's not such a good idea. > > I'm not trying to make more work for Amy. The immediate goal probably isn't > well served by this level of overthinking. But it would be nice to see all > this get simpler in the long term. @rnk suggested that too - I think it would definitely make things nicer, but yeah, the fact that it's in sys and not sys::fs is inconvenient. 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-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits