[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

I'm glad you're fixing this, this problem has came up in our profile traces as well.

> The only way this breaks things that I've managed to find is if a .cpp file using the PCH adds another template specialization that's not mentioned in the PCH

What is the error?

Could you please add test(s)? We need a test especially for the error you're mentioning. It has to be clear to the user that an error message in the /Yu .obj (because of a specialization) in fact occurs because usage of `-pch-instantiate-templates` in the pch.obj.

Have you investigated how MSVC handles this? Using a PCH in a OBJ is a lot faster with MSVC, so I'm guessing they have some trick.



================
Comment at: clang/lib/Sema/Sema.cpp:987
+    // OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+    if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {
+      llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
----------------
clang-format please.


================
Comment at: clang/lib/Sema/Sema.cpp:988
+    if(LangOpts.PCHInstantiateTemplates) {
+      llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+                                     StringRef(""));
----------------
Why not move this time trace scope inside `PerformPendingInstantiations()`? (and the other one at L927 too)


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

https://reviews.llvm.org/D69585