[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I'm not comfortable signing off on this, but it seems like this should be set up as a blocker for LLVM 4.0 if it isn't already.



================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2518-2523
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa<ImportDecl>(D) || isa<VarDecl>(D)) && Ctx.DeclMustBeEmitted(D) &&
-      D->getImportedOwningModule())
+  if ((isa<ImportDecl>(D) || isa<VarDecl>(D)) && D->getImportedOwningModule() &&
+      Ctx.DeclMustBeEmitted(D))
     return false;
 
----------------
It's not locally obvious why the order matters.  Can you add a comment explaining why you need to check getImportedOwningModule first?  It might be worth splitting Ctx.DeclMustBeEmitted into its own; e.g.,
```
// An ImportDecl or VarDecl imported from a module will get emitted when
// we import the relevant module.
if ((isa<ImportDecl>(D) || isa<VarDecl>(D)) && D->getImportedOwningModule())
  // Only check DeclMustBeEmitted if D has been fully imported, since it may
  // emit D as a side effect.
  if (Ctx.DeclMustBeEmitted(D))
    return false;
```
but anything that prevents someone from swapping the conditions when they refactor this would be good enough I think.


https://reviews.llvm.org/D29753