[PATCH] D59366: [NewPM][TimePasses] allow -time-passes reporting into a custom stream

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev marked 3 inline comments as done.
fedor.sergeev added a comment.

Thanks for review!



================
Comment at: lib/IR/PassTimingInfo.cpp:185
+void TimePassesHandler::setOutStream(raw_ostream &Out) {
+  assert(Enabled &&
+         "cant set output stream for a disabled time-passes handler");
----------------
philip.pfaffe wrote:
> I think this shouldn't be an error. If someone changes the stream for a disabled timer, so what?
There is no way to  reenable the timer. So I thought setting the stream for disabled timer means some confusion.... I'm not sure about this


================
Comment at: lib/IR/PassTimingInfo.cpp:190
+
+void TimePassesHandler::print() {
+  if (!Enabled)
----------------
philip.pfaffe wrote:
> Calling this now flushes all timer data. Is that worth a comment?
The comment is there in the header. It was a "declared" functionality both in legacy and new interfaces, but it actually was never implemented.
Catched this when writing the unit-test.
Btw, I'm adding a check for this into the unit-test, and also preparing a separate patch that adds this flushing for legacy.


================
Comment at: lib/IR/PassTimingInfo.cpp:193
+    return;
+  TG.print(OutStream ? *OutStream : *CreateInfoOutputFile());
+  TG.clear();
----------------
philip.pfaffe wrote:
> Isn't that a use-after-free? Is the stream guaranteed to survive the call to operator*?
Temporaries are cleaned up at the end of a full-expression, in this case - afteri a TG.print call.



================
Comment at: unittests/IR/TimePassesTest.cpp:18
+#include <llvm/IR/PassTimingInfo.h>
+#include <llvm/Support/raw_ostream.h>
+
----------------
philip.pfaffe wrote:
> Are the gmock and Error includes actually used?
yeah, this was copied from some other test, will clean it out.


================
Comment at: unittests/IR/TimePassesTest.cpp:58
+
+  TimePasses.reset();
+
----------------
philip.pfaffe wrote:
> Maybe leave a comment why this is here.
will add a bit more  code here, with comments...


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59366