[PATCH] D52954: Annotate timeline in Instruments with passes and other timed regions.
Justin Bogner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 13 16:35:14 PST 2019
bogner accepted this revision.
bogner added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jdoerfert.
Is there a more generic term for this than "signpost"? If so, it might be nice to use that to avoid confusion when/if other implementations show up. If not, that's fine.
Overall, looks reasonable to me, other than a few nitpicks.
Comment at: cmake/modules/HandleLLVMOptions.cmake:893
+option(LLVM_SUPPORT_XCODE_SIGNPOSTS "Enable support for Xcode signposts" OFF)
Can we detect when this is available rather than make it an option?
Comment at: include/llvm/Support/Timer.h:16
Public headers can't include config.h. We don't install it, so folks using llvm as a library can't see it. That said, do we even need this include?
Comment at: lib/Support/Signposts.cpp:84-87
+ Impl = nullptr;
+ Impl = new SignpostEmitterImpl();
+#endif // if HAVE_ANY_SIGNPOST_IMPL
Better to do #if/#else here, in case any compilers warn about the redundant assignment (I'm not sure if they do)
Comment at: lib/Support/Signposts.cpp:98-99
+ if (Impl == nullptr)
+ return false;
+ return Impl->isEnabled();
Can Impl ever be null if HAVE_ANY_SIGNPOST_IMPL? I think we can remove these checks.
CHANGES SINCE LAST ACTION
More information about the llvm-commits