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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits