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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
