[PATCH] D132421: [HLSL] Support PCH for cc1 mode

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:50
+    if (S.LookupQualifiedName(Result, HLSLNamespace)) {
+      NamedDecl *Found = Result.getFoundDecl();
+      if (auto *TD = dyn_cast<ClassTemplateDecl>(Found)) {
----------------
99.9% sure this is safe because we're looking up a tag name and I can't think of any way that might return multiple results for the same identifier, but bringing it up just in case anyone else knows of some obscure extension where that's possible.


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:55
+      } else
+        PrevDecl = dyn_cast<CXXRecordDecl>(Found);
+      assert(PrevDecl && "Unexpected lookup result type.");
----------------
Is it possible that this finds a different kind of tag, like an enumeration?


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:85-86
                     AccessSpecifier Access = AccessSpecifier::AS_private) {
+    if (Record->isCompleteDefinition())
+      return *this;
     assert(Record->isBeingDefined() &&
----------------
A downside to this pattern is that we need to repeat the "if it's a complete definition, don't do anything" predicate in basically any mutating member function. Getting that wrong will lead to hard-to-debug issues with PCH, as I understand it. But I don't think there's a cleaner way to do that without doing far more invasive changes.


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:378
+  if (S.LookupQualifiedName(Result, AST.getTranslationUnitDecl()))
+    PrevDecl = Result.getAsSingle<NamespaceDecl>();
+  HLSLNamespace = NamespaceDecl::Create(AST, AST.getTranslationUnitDecl(),
----------------
How certain are you that there's only one result possible here?


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:387
+  // Force external decls in the HLSL namespace to load from the PCH.
+  HLSLNamespace->getCanonicalDecl()->decls_begin();
   defineTrivialHLSLTypes();
----------------



================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:459-461
+  ASTContext &AST = SemaPtr->getASTContext();
+  IdentifierInfo &II = AST.Idents.get("Resource", tok::TokenKind::identifier);
+  LookupResult Result(*SemaPtr, &II, SourceLocation(), Sema::LookupTagName);
----------------
We made a lookup result but then do nothing with it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132421