<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 2, 2014 at 4:25 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It looks like this went out for review but was never reviewed? If so,<br>
why did you commit?</blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im HOEnZb">On Wed, Jul 2, 2014 at 4:08 PM, Larisse Voufo <<a href="mailto:lvoufo@google.com">lvoufo@google.com</a>> wrote:<br>
</div><div class="HOEnZb"><div class="h5">> Author: lvoufo<br>
> Date: Wed Jul 2 18:08:34 2014<br>
> New Revision: 212233<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=212233&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=212233&view=rev</a><br>
> Log:<br>
> 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.<br>
> static int &foo() {<br>
> struct Foo { };<br>
> return x<Foo>;<br>
> }<br>
><br>
> Modified:<br>
> cfe/trunk/lib/AST/Decl.cpp<br>
><br>
> Modified: cfe/trunk/lib/AST/Decl.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=212233&r1=212232&r2=212233&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=212233&r1=212232&r2=212233&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/AST/Decl.cpp (original)<br>
> +++ cfe/trunk/lib/AST/Decl.cpp Wed Jul 2 18:08:34 2014<br>
> @@ -471,6 +471,58 @@ static void mergeTemplateLV(LinkageInfo<br>
> LV.mergeExternalVisibility(argsLV);<br>
> }<br>
><br>
> +/// Should we consider visibility associated with the template<br>
> +/// arguments and parameters of the given variable template<br>
> +/// specialization? As usual, follow class template specialization<br>
> +/// logic up to initialization.<br>
> +static bool shouldConsiderTemplateVisibility(<br>
> + const VarTemplateSpecializationDecl *spec,<br>
> + LVComputationKind computation) {<br>
> + // Include visibility from the template parameters and arguments<br>
> + // only if this is not an explicit instantiation or specialization<br>
> + // with direct explicit visibility (and note that implicit<br>
> + // instantiations won't have a direct attribute).<br>
> + if (!spec->isExplicitInstantiationOrSpecialization())<br>
> + return true;<br>
> +<br>
> + // An explicit variable specialization is an independent, top-level<br>
> + // declaration. As such, if it has an explicit visibility attribute,<br>
> + // that must directly express the user's intent, and we should honor<br>
> + // it.<br>
> + if (spec->isExplicitSpecialization() &&<br>
> + hasExplicitVisibilityAlready(computation))<br>
> + return false;<br>
> +<br>
> + return !hasDirectVisibilityAttribute(spec, computation);<br>
> +}<br>
> +<br>
> +/// Merge in template-related linkage and visibility for the given<br>
> +/// variable template specialization. As usual, follow class template<br>
> +/// specialization logic up to initialization.<br>
> +static void mergeTemplateLV(LinkageInfo &LV,<br>
> + const VarTemplateSpecializationDecl *spec,<br>
> + LVComputationKind computation) {<br>
> + bool considerVisibility = shouldConsiderTemplateVisibility(spec, computation);<br>
> +<br>
> + // Merge information from the template parameters, but ignore<br>
> + // visibility if we're only considering template arguments.<br>
> +<br>
> + VarTemplateDecl *temp = spec->getSpecializedTemplate();<br>
> + LinkageInfo tempLV =<br>
> + getLVForTemplateParameterList(temp->getTemplateParameters(), computation);<br>
> + LV.mergeMaybeWithVisibility(tempLV,<br>
> + considerVisibility && !hasExplicitVisibilityAlready(computation));<br>
> +<br>
> + // Merge information from the template arguments. We ignore<br>
> + // template-argument visibility if we've got an explicit<br>
> + // instantiation with a visibility attribute.<br>
> + const TemplateArgumentList &templateArgs = spec->getTemplateArgs();<br>
> + LinkageInfo argsLV = getLVForTemplateArgumentList(templateArgs, computation);<br>
> + if (considerVisibility)<br>
> + LV.mergeVisibility(argsLV);<br>
> + LV.mergeExternalVisibility(argsLV);<br>
> +}<br>
> +<br>
> static bool useInlineVisibilityHidden(const NamedDecl *D) {<br>
> // FIXME: we should warn if -fvisibility-inlines-hidden is used with c.<br>
> const LangOptions &Opts = D->getASTContext().getLangOpts();<br>
> @@ -662,6 +714,14 @@ static LinkageInfo getLVForNamespaceScop<br>
> // C99 6.2.2p4 and propagating the visibility attribute, so we don't have<br>
> // to do it here.<br>
><br>
> + // As per function and class template specializations (below),<br>
> + // consider LV for the template and template arguments. We're at file<br>
> + // scope, so we do not need to worry about nested specializations.<br>
> + if (const VarTemplateSpecializationDecl *spec<br>
> + = dyn_cast<VarTemplateSpecializationDecl>(Var)) {<br>
> + mergeTemplateLV(LV, spec, computation);<br>
> + }<br>
> +<br>
> // - a function, unless it has internal linkage; or<br>
> } else if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D)) {<br>
> // In theory, we can modify the function's LV by the LV of its<br>
> @@ -869,6 +929,10 @@ static LinkageInfo getLVForClassMember(c<br>
><br>
> // Static data members.<br>
> } else if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {<br>
> + if (const VarTemplateSpecializationDecl *spec<br>
> + = dyn_cast<VarTemplateSpecializationDecl>(VD))<br>
> + mergeTemplateLV(LV, spec, computation);<br>
> +<br>
> // Modify the variable's linkage by its type, but ignore the<br>
> // type's visibility unless it's a definition.<br>
> LinkageInfo typeLV = getLVForType(*VD->getType(), computation);<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>