[PATCH] D123677: [PassManager] Implement DOTGraphTraitsViewer under NPM

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

No need to reorganize your patches now, but please keep it in mind for your next contributions.

LG too commit, or update it to introduce NPM passes.



================
Comment at: llvm/include/llvm/Analysis/DOTGraphTraitsPass.h:32
+template <typename GraphT>
+static void runViewerImpl(Function &F, GraphT &Graph, StringRef Name,
+                          bool IsSimple) {
----------------
Meinersbur wrote:
> myhsu wrote:
> > I think `static` is redundant in this case.
> > Also, in LLVM, "XXXImpl" sounds more like a name for internal / implementation-specific code, but this function is global. I would recommend changing this (and runPrinterImpl) into something more general, like "viewGraphForFunction".
> I don't see static being redundant here, it changes it to internal linkage. A template function is implicitly inline. Without `static`, every TU including this file will have to emit it, influencing compilation time.
This actually isn't a change request, just wanted to point out that `static` has an effect. Inline non-static allows the linker to select just one of the definitions whereas `static inline` there might be multiple identical definitions the the final artifact. No opinion on which is better.


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

https://reviews.llvm.org/D123677