[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Richard Smith via cfe-commits]

rsmith added inline comments.


> SemaDecl.cpp:3692-3693
> +      // Demote the newly parsed definition to a fake declaration.
> +      // FIXME: Sema::AddInitializationToDecl still allows two definitions,
> +      // which make the AST variants inconsistent.
> +      assert (Def != New && "There is only one definition!");

We always get here after calling that, don't we? So we only briefly have two definitions, between the point at which we attach the definition and now. If that's what you're referring to here, this comment could be clearer about it.

> SemaDecl.cpp:3695
> +      assert (Def != New && "There is only one definition!");
> +      New->demoteThisDefinitionToDeclaration();
>      } else if (Old->isStaticDataMember() &&

You should also call `makeMergedDefinitionVisible(Def, New->getLocation())` here (and also call it for `Def`'s enclosing template if this is a variable template definition) to make the prior definition visible.

> ASTReaderDecl.cpp:3083-3087
> +    // Fast track.
> +    if (PrevVD->isThisDeclarationADefinition()) {
> +      VD->demoteThisDefinitionToDeclaration();
> +      return;
> +    }

I don't think this is worthwhile, since it's the first thing the loop below will check.

> ASTReaderDecl.cpp:3089
> +
> +    for (VarDecl *CurD = D->First; CurD; CurD = CurD->getPreviousDecl())
> +      if (CurD->isThisDeclarationADefinition()) {

You should start at `PrevVD` not at `D->First`. This loop will currently only iterate once.

> ASTReaderDecl.cpp:3090
> +    for (VarDecl *CurD = D->First; CurD; CurD = CurD->getPreviousDecl())
> +      if (CurD->isThisDeclarationADefinition()) {
> +        // If we found another definition on the chain, demote the current one.

You can also bail out early and demote the current definition if you reach a previous demoted definition. That would reduce this from quadratic-time to linear-time.

> v.g.vassilev wrote in ASTWriterDecl.cpp:896-897
> I thought we might need this for c-style `void f(struct S arg)`-like constructs where we might need to demote if we merge ParmVarDecls.

We'll still have only one `ParmVarDecl` per `FunctionDecl`, and no-one cares whether a `ParmVarDecl` is a definition. Also, you assert that the flag is always false in this case below.

https://reviews.llvm.org/D24508