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

[Richard Smith via cfe-commits]

rsmith added a comment.

This looks like it's going in the right direction.



> Decl.cpp:2269-2272
> +      // If we have hit a point where the user provided a specialization of
> +      // this template, we're done looking.
> +      if (VarTemplate->isMemberSpecialization())
> +        break;

I think we need a similar check in the static data member case above.

> Decl.cpp:2278
> +            !isTemplateInstantiation(getTemplateSpecializationKind())) &&
> +           "couldn't find pattern for enum instantiation");
> +

enum?

> rsmith wrote in SemaTemplate.cpp:509
> `else if` doesn't make sense here -- we either need to produce a diagnostic on all paths through here, or suppress the notes if we didn't produce a diagnostic.

This function still appears to be able to return true (indicating to the caller that a diagnostic was produced) without actually producing a diagnostic.

> ASTWriterDecl.cpp:896-897
>    Record.push_back(D->getInitStyle());
> +  Record.push_back(D->isThisDeclarationADemotedDefinition());
>    if (!isa<ParmVarDecl>(D)) {
>      Record.push_back(D->isExceptionVariable());

Sink this flag into the "not for `ParmVarDecl`" block below.

> ASTWriterDecl.cpp:1965
> +  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // TSCSpec
> +  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // InitStyle
> +  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsThisDeclarationADemotedDefinition

Hmm. The width of the `InitStyle` field is definitely wrong right now, but should be fixed separately from this change. It looks like we don't hit this today because we don't use this abbreviation for a variable with an initializer. In addition to fixing the width of this field, we should also remove the `getInit() == nullptr` check when selecting the abbreviation in `ASTDeclWriter::VisitVarDecl`.

https://reviews.llvm.org/D24508