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