[PATCH] D81078: [Support] Use outs() in ToolOutputFile

Wed Jun 3 04:53:59 PDT 2020

jhenderson added a comment.

Thanks for looking at this. Having given it more thought, any side-effects this fix will have must be in the realm of bug fixes - if people were mixing printing to stdout from two different sources, they wouldn't expect potential stream interleaving after all.

Perhaps it's worth a simple unit test that shows that the result of `ToolOutputFile.os()` is the same as `outs()`, by comparing addresses? Not sure though.

================
Comment at: llvm/include/llvm/Support/raw_ostream.h:501
+/// like: outs() << "foo" << "bar";
+raw_fd_ostream &outs();

----------------
I'm not sure I'm following the reason for changing from the base class here? I'm guessing it's to fit in well with `ToolOutputFile`'s usage, but coudl that be changed to store a pointer to and return a `raw_ostream` easily?

================
Comment at: llvm/lib/Support/ToolOutputFile.cpp:23
   // Arrange for the file to be deleted if the process is killed.
-  if (Filename != "-")
+  if (isStdout(Filename))
     sys::RemoveFileOnSignal(Filename);
----------------
This logic seems to have been inverted?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81078/new/

https://reviews.llvm.org/D81078