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

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev 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);
----------------
philip.pfaffe wrote:
> 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.
> 
ok


================
Comment at: unittests/IR/TimePassesTest.cpp:29
+
+namespace {
+struct Pass1 : public ModulePass {
----------------
philip.pfaffe wrote:
> 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.
*that* is a completely different anonymous namespace, which is not nested into llvm.

this one:
namespace llvm {
   namespace {
      ... passes here
   }
}
is a namespace for Passes, which are required to be llvm::something, otherwise INITIALIZE_PASS does not seem to work.

and the next one:
namespace {
   ... tests here
}
is a namespace for tests, which can be whatever you want.
I can remove the latter and put everything (passes and tests) inside the  former (nested inside llvm::) one.
I dont see it as a good solution, but I dont have strong feelings about that. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59416