[PATCH] D59416: [Legacy][TimePasses] allow -time-passes reporting into a custom stream

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added inline comments.


================
Comment at: include/llvm/IR/PassTimingInfo.h:32
 /// If -time-passes has been specified, report the timings immediately and then
-/// reset the timers to zero.
-void reportAndResetTimings();
+/// reset the timers to zero. By default it uses info-output-file stream.
+void reportAndResetTimings(raw_ostream *OutStream = nullptr);
----------------
fedor.sergeev wrote:
> fedor.sergeev wrote:
> > philip.pfaffe wrote:
> > > Sorry, this second nit got lost: Is it clear from context what `info-output-file` is? Otherwise reference `CreateInfoOutputFile()` instead.
> > Well, both of these would require a search through sources if you do not know what it is.
> > info-output-file is a command-line level control, CreateInfoOutputFile is an API control.
> > I really do not have any preferences here.
> Will it be more clear if I say "By default it uses file stream specified by -info-output-file" ?
Technically that's not correct though, per default it uses CreateInfoOutputFile as far as the API is concerned. That //that// then reads the command line option is still one more hop.



================
Comment at: unittests/IR/TimePassesTest.cpp:29
+
+namespace {
+struct Pass1 : public ModulePass {
----------------
fedor.sergeev wrote:
> philip.pfaffe wrote:
> > Is this one still necessary?
> well... as I see it is quite common through the unittests to use anonymous namespaces to hide test stuff
> (I gather to avoid any clashes with LLVM interfaces).
> Say, LegacyPassManagerTest does pretty much the same.
> Its just that weird requirement of INITIALIZE_PASS for legacy passes to be inside namespace llvm that requires
> extra namespace busywork.
> 
> I'm open for any suggestions here.
Sure, you're opening another anonymous namespace 30 lines down though.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59416