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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to