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

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.


================
Comment at: clang/include/clang/Sema/HLSLExternalSemaSource.h:58
+/// them before we initialize the ExternalSemaSource base class.
+struct ChainedHLSLExternalSemaSourceMembers {
+  ChainedHLSLExternalSemaSourceMembers(ExternalSemaSource *ExtSema)
----------------
IIUC, this code just exists to make sure that the `ASTReader` deserializes before the external sema source starts adding things. Is that correct?

If so, you don't need to do this, instead you can just add this code to `InitializeSema()` to force the `ASTReader` to de-serialize the decls:

```
// If the translation unit has external storage force external decls to load.
if (AST.getTranslationUnitDecl()->hasExternalLexicalStorage())
    (void)AST.getTranslationUnitDecl()->decls_begin();
```


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:297
+  }
+
   IdentifierInfo &HLSL = AST.Idents.get("hlsl", tok::TokenKind::identifier);
----------------
I think the core of what you'e doing here is not far off, but I think this pattern doesn't scale or cover all the use cases that matter.

The cases that come to my mind are:
1) PCH has a forward declaration, source doesn't use the type so we shouldn't complete the type.
2) PCH has a forward declaration, source uses the type, so we need to generate a complete decl.
3) PCH has a complete declaration, so we should re-use the declaration and not need to do anything.

We also need to keep in mind that we're going to keep adding more and more types, so this pattern where each new type needs to add code to lookup is going to be a challenge. It is also going to be a challenge to manage the complexity of PCH defines one thing but not another.

One thing you should look at is how clang handles the following C++ code:

```
template<typename T>
class Foo;

template<typename T>
class Foo {
  T Val;
};
```

This results in two decls. One for the forward decl and the second for the definition. The definition lists the forward declaration as the previous decl. We should do that here.

When we generate the HLSL namespace, we can do a lookup and if we find a previous decl, set the previous decl.

When we generate any of the builtin types, we can lookup the previous decl, and annotate as the previous decl. We'll can also handle the case where a decl from the PCH is complete. If the PCH provides a complete decl, we can skip generating any decls for it, and the lookups should just work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132421