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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

In D123677#3458779 <https://reviews.llvm.org/D123677#3458779>, @YangKeao wrote:

>> `DOTGraphTraitsPrinter` is not used in this patch.
>
> It is actually used in the D123678 <https://reviews.llvm.org/D123678> , by `ScopPrinter`

Usually patches organized self-contained units of changes, which includes not introducing dead code. If you need the code only in D123678 <https://reviews.llvm.org/D123678>, introduce the code there. There is a possibility to have a refactoring/NFC patch to make the diff of a followup patch more readable, but those also do not introduce dead code.

A possibility to not introduce dead code would be to use the code in NPM versions of DomPrinter etc, which is why I was suggesting it.



================
Comment at: llvm/include/llvm/Analysis/DOTGraphTraitsPass.h:32
+template <typename GraphT>
+static void runViewerImpl(Function &F, GraphT &Graph, StringRef Name,
+                          bool IsSimple) {
----------------
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.


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

https://reviews.llvm.org/D123677