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

[Hans Wennborg via cfe-commits]

Richard, can you take a look when you have a moment? The PR is marked
as a release blocker.

On Thu, Feb 9, 2017 at 1:54 PM, Duncan P. N. Exon Smith via
Phabricator via cfe-commits <cfe-commits at lists.llvm.org> wrote:
> 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
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits