On Wed, Jul 2, 2014 at 4:44 PM, Richard Smith <[email protected]> wrote: > On Wed, Jul 2, 2014 at 4:25 PM, Eric Christopher <[email protected]> wrote: >> >> It looks like this went out for review but was never reviewed? If so, >> why did you commit? > > > Agreed, if a patch doesn't meet the "obvious" bar and needs pre-commit > review, it shouldn't be committed until that review happens. In this case, I > think the patch actually does meet the "obvious" bar: it's a trivial > extension of our existing mechanism for other flavors of templates to also > cover variable templates. >
Cool. Mostly that if it goes out for review it should stay out for review until someone acks it. -eric > One additional post-commit review comment (in addition to the request for a > testcase): perhaps it's time to refactor this code to have common code for > handling any kind of template, rather than having distinct code for each > different flavor of template. > >> On Wed, Jul 2, 2014 at 4:08 PM, Larisse Voufo <[email protected]> wrote: >> > Author: lvoufo >> > Date: Wed Jul 2 18:08:34 2014 >> > New Revision: 212233 >> > >> > URL: http://llvm.org/viewvc/llvm-project?rev=212233&view=rev >> > Log: >> > Fix linkage bug that miscompiled variable templates instantiated from >> > similarly named local types. In essence, this bug ensures that the x<Foo> >> > specialization in function foo() defined as follows differs between two >> > distinct translation units. >> > static int &foo() { >> > struct Foo { }; >> > return x<Foo>; >> > } >> > >> > Modified: >> > cfe/trunk/lib/AST/Decl.cpp >> > >> > Modified: cfe/trunk/lib/AST/Decl.cpp >> > URL: >> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=212233&r1=212232&r2=212233&view=diff >> > >> > ============================================================================== >> > --- cfe/trunk/lib/AST/Decl.cpp (original) >> > +++ cfe/trunk/lib/AST/Decl.cpp Wed Jul 2 18:08:34 2014 >> > @@ -471,6 +471,58 @@ static void mergeTemplateLV(LinkageInfo >> > LV.mergeExternalVisibility(argsLV); >> > } >> > >> > +/// Should we consider visibility associated with the template >> > +/// arguments and parameters of the given variable template >> > +/// specialization? As usual, follow class template specialization >> > +/// logic up to initialization. >> > +static bool shouldConsiderTemplateVisibility( >> > + const VarTemplateSpecializationDecl >> > *spec, >> > + LVComputationKind computation) { >> > + // Include visibility from the template parameters and arguments >> > + // only if this is not an explicit instantiation or specialization >> > + // with direct explicit visibility (and note that implicit >> > + // instantiations won't have a direct attribute). >> > + if (!spec->isExplicitInstantiationOrSpecialization()) >> > + return true; >> > + >> > + // An explicit variable specialization is an independent, top-level >> > + // declaration. As such, if it has an explicit visibility attribute, >> > + // that must directly express the user's intent, and we should honor >> > + // it. >> > + if (spec->isExplicitSpecialization() && >> > + hasExplicitVisibilityAlready(computation)) >> > + return false; >> > + >> > + return !hasDirectVisibilityAttribute(spec, computation); >> > +} >> > + >> > +/// Merge in template-related linkage and visibility for the given >> > +/// variable template specialization. As usual, follow class template >> > +/// specialization logic up to initialization. >> > +static void mergeTemplateLV(LinkageInfo &LV, >> > + const VarTemplateSpecializationDecl *spec, >> > + LVComputationKind computation) { >> > + bool considerVisibility = shouldConsiderTemplateVisibility(spec, >> > computation); >> > + >> > + // Merge information from the template parameters, but ignore >> > + // visibility if we're only considering template arguments. >> > + >> > + VarTemplateDecl *temp = spec->getSpecializedTemplate(); >> > + LinkageInfo tempLV = >> > + getLVForTemplateParameterList(temp->getTemplateParameters(), >> > computation); >> > + LV.mergeMaybeWithVisibility(tempLV, >> > + considerVisibility && >> > !hasExplicitVisibilityAlready(computation)); >> > + >> > + // Merge information from the template arguments. We ignore >> > + // template-argument visibility if we've got an explicit >> > + // instantiation with a visibility attribute. >> > + const TemplateArgumentList &templateArgs = spec->getTemplateArgs(); >> > + LinkageInfo argsLV = getLVForTemplateArgumentList(templateArgs, >> > computation); >> > + if (considerVisibility) >> > + LV.mergeVisibility(argsLV); >> > + LV.mergeExternalVisibility(argsLV); >> > +} >> > + >> > static bool useInlineVisibilityHidden(const NamedDecl *D) { >> > // FIXME: we should warn if -fvisibility-inlines-hidden is used with >> > c. >> > const LangOptions &Opts = D->getASTContext().getLangOpts(); >> > @@ -662,6 +714,14 @@ static LinkageInfo getLVForNamespaceScop >> > // C99 6.2.2p4 and propagating the visibility attribute, so we >> > don't have >> > // to do it here. >> > >> > + // As per function and class template specializations (below), >> > + // consider LV for the template and template arguments. We're at >> > file >> > + // scope, so we do not need to worry about nested specializations. >> > + if (const VarTemplateSpecializationDecl *spec >> > + = dyn_cast<VarTemplateSpecializationDecl>(Var)) { >> > + mergeTemplateLV(LV, spec, computation); >> > + } >> > + >> > // - a function, unless it has internal linkage; or >> > } else if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D)) >> > { >> > // In theory, we can modify the function's LV by the LV of its >> > @@ -869,6 +929,10 @@ static LinkageInfo getLVForClassMember(c >> > >> > // Static data members. >> > } else if (const VarDecl *VD = dyn_cast<VarDecl>(D)) { >> > + if (const VarTemplateSpecializationDecl *spec >> > + = dyn_cast<VarTemplateSpecializationDecl>(VD)) >> > + mergeTemplateLV(LV, spec, computation); >> > + >> > // Modify the variable's linkage by its type, but ignore the >> > // type's visibility unless it's a definition. >> > LinkageInfo typeLV = getLVForType(*VD->getType(), computation); >> > >> > >> > _______________________________________________ >> > cfe-commits mailing list >> > [email protected] >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
