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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added a comment.

Couple of nits inline, otherwise looks great!



================
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");
----------------
I think this shouldn't be an error. If someone changes the stream for a disabled timer, so what?


================
Comment at: lib/IR/PassTimingInfo.cpp:190
+
+void TimePassesHandler::print() {
+  if (!Enabled)
----------------
Calling this now flushes all timer data. Is that worth a comment?


================
Comment at: lib/IR/PassTimingInfo.cpp:193
+    return;
+  TG.print(OutStream ? *OutStream : *CreateInfoOutputFile());
+  TG.clear();
----------------
Isn't that a use-after-free? Is the stream guaranteed to survive the call to operator*?


================
Comment at: tools/opt/NewPMDriver.cpp:18
 #include "PassPrinters.h"
+#include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringRef.h"
----------------
Unrelated change.


================
Comment at: tools/opt/NewPMDriver.cpp:262
+
+  // Setting up standard instrumentations.
   PassInstrumentationCallbacks PIC;
----------------
Unrelated change.


================
Comment at: unittests/IR/TimePassesTest.cpp:18
+#include <llvm/IR/PassTimingInfo.h>
+#include <llvm/Support/raw_ostream.h>
+
----------------
Are the gmock and Error includes actually used?


================
Comment at: unittests/IR/TimePassesTest.cpp:31
+using testing::WithArgs;
+using testing::_;
+
----------------
Do you need any of these?


================
Comment at: unittests/IR/TimePassesTest.cpp:58
+
+  TimePasses.reset();
+
----------------
Maybe leave a comment why this is here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59366