[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Overall looks good. Was this tested on large software? I would also be grateful if you could run the regression tests with templight always being enabled to see if they uncover any assertions/crashes.



================
Comment at: include/clang/Driver/CC1Options.td:537
+def templight_dump : Flag<["-"], "templight-dump">,
+  HelpText<"Dump templight information to stdout">;
 def ast_dump_lookups : Flag<["-"], "ast-dump-lookups">,
----------------
Do we want to keep the templight name? I am ok with it, but I wonder if something like dump template instantiation information, or dump template profile information would be more descriptive to the newcomers.


================
Comment at: include/clang/Sema/Sema.h:7117
+
+      /// Added for Template instantiation observation
+      /// Memoization means we are _not_ instantiating a template because
----------------
Terminate the first sentence with period.


================
Comment at: include/clang/Sema/TemplateInstCallback.h:26
+public:
+  virtual ~TemplateInstantiationCallback() {}
+
----------------
Prefer `= default;`


================
Comment at: lib/Frontend/FrontendActions.cpp:29
+#include "llvm/Support/YAMLTraits.h"
+#include <iostream>
 #include <memory>
----------------
Is this header used? If not (after solving my other comments below), please remove it.


================
Comment at: lib/Frontend/FrontendActions.cpp:319
+                               const CodeSynthesisContext &Inst) override {
+    DisplayTemplightEntry<true>(std::cout, TheSema, Inst);
+  }
----------------
`std::cout` is rarely used in LLVM. Did you consider `llvm::outs`?
Also, do we want to output to the standard output or standard error? Is there other dump functionality that is being printed to the standard output?


================
Comment at: lib/Frontend/FrontendActions.cpp:357
+  template <bool BeginInstantiation>
+  static void DisplayTemplightEntry(std::ostream &Out, const Sema &TheSema,
+                                    const CodeSynthesisContext &Inst) {
----------------
Some methods still starts with uppercase letters.


================
Comment at: lib/Frontend/FrontendActions.cpp:377
+    Entry.Event = BeginInstantiation ? "Begin" : "End";
+    if (NamedDecl *NamedTemplate = dyn_cast_or_null<NamedDecl>(Inst.Entity)) {
+      llvm::raw_string_ostream OS(Entry.Name);
----------------
Use `auto *` to avoid repeating the type name.


================
Comment at: lib/FrontendTool/ExecuteCompilerInvocation.cpp:261
+
+}
----------------
Add namespace closing comment.


================
Comment at: lib/Sema/Sema.cpp:40
 #include "clang/Sema/TemplateDeduction.h"
+#include "clang/Sema/TemplateInstCallback.h"
 #include "llvm/ADT/DenseMap.h"
----------------
Do you need to add this include?


================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:204
+       
+  case Memoization:
+    break;
----------------
This function should never be called with `Memoization`? Maybe this worth a comment?


https://reviews.llvm.org/D5767