[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)
----------------
python3kgae wrote:
> beanz wrote:
> > 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();
> > ```
> Still need this to make sure HLSLSema and ExternalSema are initialized before  MultiplexExternalSemaSource.
In FrontendAction, where you are creating the Chained source, you can instead create a Multiplex source, and put the PCH external source in as the first source and the HLSL one second. The PCH will get initialized first, the HLSL one can force the PCH one to populate the decls.


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:314
+
+  if (!HLSLNamespace) {
+    HLSLNamespace = NamespaceDecl::Create(AST, AST.getTranslationUnitDecl(),
----------------
Rather than reusing the namespace you can set the PCH one as the previous decl for the one you're declaring here. That will result in them basically being merged because Namespaces can have multiple decls.

The advantage to this is that the PCH namespace will be marked in the AST as a deserialized decl but the HLSL one won't. It will make debugging and AST visualization easier.


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:411
+    Completions.insert(std::make_pair(
+        Decl, std::bind(&HLSLExternalSemaSource::completeBufferType, this,
+                        std::placeholders::_1)));
----------------
If the decl you put in here is the canonical decl, and you do the lookup by canonical decl. The conflicting decls shouldn't be an issue. That does require that you mark the previous decl so that they are known to be the same declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132421