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@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@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
