On Thu, Oct 4, 2012 at 6:20 PM, Sean Silva <span dir="ltr"><<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">+ if (*IsInline && II && II->getName().startswith("__atomic") &&<br>
+ S.getSourceManager().isInSystemHeader(Loc)) {<br>
+ // libstdc++4.6's <atomic> reopens a non-inline namespace as inline, and<br>
+ // expects that to affect the earlier declaration.<br>
+ for (NamespaceDecl *NS = PrevNS->getMostRecentDecl(); NS;<br>
+ NS = NS->getPreviousDecl())<br>
+ NS->setInline(*IsInline);<br>
+ // Patch up the lookup table for the containing namespace. This<br>
isn't really<br>
+ // correct, but it's good enough for this particular case.<br>
<br>
</div>Could you call out the fact that this is a hack a bit more loudly? It<br>
all seems quite casual with the short explanation tucked away nice and<br>
tidy inside the `if`. Something like `// HACK: ....` above the `if`?<br></blockquote><div><br></div><div>Sounds sensible to me. r165286.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, it's probably worth saying under what conditions the hack should<br>
be pulled. Otherwise there's a risk that it will get pulled<br>
inappropriately.<br></blockquote><div><br></div><div>What do you mean by pulled? Dropped?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
btw, I love the #ifdef BE_THE_HEADER thing :)<br>
<span class="HOEnZb"><font color="#888888"><br>
-- Sean Silva<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Thu, Oct 4, 2012 at 6:13 PM, Richard Smith<br>
<<a href="mailto:richard-llvm@metafoo.co.uk">richard-llvm@metafoo.co.uk</a>> wrote:<br>
> Author: rsmith<br>
> Date: Thu Oct 4 17:13:39 2012<br>
> New Revision: 165263<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=165263&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=165263&view=rev</a><br>
> Log:<br>
> Egriegious hack to support libstdc++4.6's broken <atomic> header, which defines<br>
> a non-inline namespace, then reopens it as inline to try to add its symbols to<br>
> the surrounding namespace. In this one special case, permit the namespace to be<br>
> reopened as inline, and patch up the name lookup tables to match.<br>
><br>
> Added:<br>
> cfe/trunk/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp<br>
> Modified:<br>
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> cfe/trunk/lib/Sema/SemaDeclCXX.cpp<br>
> cfe/trunk/test/CXX/dcl.dcl/basic.namespace/namespace.def/p7.cpp<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=165263&r1=165262&r2=165263&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=165263&r1=165262&r2=165263&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Oct 4 17:13:39 2012<br>
> @@ -781,7 +781,7 @@<br>
> def err_static_assert_failed : Error<"static_assert failed %0">;<br>
><br>
> def warn_inline_namespace_reopened_noninline : Warning<<br>
> - "inline namespace cannot be re-opened as a non-inline namespace">;<br>
> + "inline namespace cannot be reopened as a non-inline namespace">;<br>
> def err_inline_namespace_mismatch : Error<<br>
> "%select{|non-}0inline namespace "<br>
> "cannot be reopened as %select{non-|}0inline">;<br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=165263&r1=165262&r2=165263&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=165263&r1=165262&r2=165263&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Thu Oct 4 17:13:39 2012<br>
> @@ -5399,7 +5399,42 @@<br>
> // Namespace Handling<br>
> //===----------------------------------------------------------------------===//<br>
><br>
> +/// \brief Diagnose a mismatch in 'inline' qualifiers when a namespace is<br>
> +/// reopened.<br>
> +static void DiagnoseNamespaceInlineMismatch(Sema &S, SourceLocation KeywordLoc,<br>
> + SourceLocation Loc,<br>
> + IdentifierInfo *II, bool *IsInline,<br>
> + NamespaceDecl *PrevNS) {<br>
> + assert(*IsInline != PrevNS->isInline());<br>
> +<br>
> + if (*IsInline && II && II->getName().startswith("__atomic") &&<br>
> + S.getSourceManager().isInSystemHeader(Loc)) {<br>
> + // libstdc++4.6's <atomic> reopens a non-inline namespace as inline, and<br>
> + // expects that to affect the earlier declaration.<br>
> + for (NamespaceDecl *NS = PrevNS->getMostRecentDecl(); NS;<br>
> + NS = NS->getPreviousDecl())<br>
> + NS->setInline(*IsInline);<br>
> + // Patch up the lookup table for the containing namespace. This isn't really<br>
> + // correct, but it's good enough for this particular case.<br>
> + for (DeclContext::decl_iterator I = PrevNS->decls_begin(),<br>
> + E = PrevNS->decls_end(); I != E; ++I)<br>
> + if (NamedDecl *ND = dyn_cast<NamedDecl>(*I))<br>
> + PrevNS->getParent()->makeDeclVisibleInContext(ND);<br>
> + return;<br>
> + }<br>
><br>
> + if (PrevNS->isInline())<br>
> + // The user probably just forgot the 'inline', so suggest that it<br>
> + // be added back.<br>
> + S.Diag(Loc, diag::warn_inline_namespace_reopened_noninline)<br>
> + << FixItHint::CreateInsertion(KeywordLoc, "inline ");<br>
> + else<br>
> + S.Diag(Loc, diag::err_inline_namespace_mismatch)<br>
> + << IsInline;<br>
> +<br>
> + S.Diag(PrevNS->getLocation(), diag::note_previous_definition);<br>
> + *IsInline = PrevNS->isInline();<br>
> +}<br>
><br>
> /// ActOnStartNamespaceDef - This is called at the start of a namespace<br>
> /// definition.<br>
> @@ -5449,21 +5484,9 @@<br>
><br>
> if (PrevNS) {<br>
> // This is an extended namespace definition.<br>
> - if (IsInline != PrevNS->isInline()) {<br>
> - // inline-ness must match<br>
> - if (PrevNS->isInline()) {<br>
> - // The user probably just forgot the 'inline', so suggest that it<br>
> - // be added back.<br>
> - Diag(Loc, diag::warn_inline_namespace_reopened_noninline)<br>
> - << FixItHint::CreateInsertion(NamespaceLoc, "inline ");<br>
> - } else {<br>
> - Diag(Loc, diag::err_inline_namespace_mismatch)<br>
> - << IsInline;<br>
> - }<br>
> - Diag(PrevNS->getLocation(), diag::note_previous_definition);<br>
> -<br>
> - IsInline = PrevNS->isInline();<br>
> - }<br>
> + if (IsInline != PrevNS->isInline())<br>
> + DiagnoseNamespaceInlineMismatch(*this, NamespaceLoc, Loc, II,<br>
> + &IsInline, PrevNS);<br>
> } else if (PrevDecl) {<br>
> // This is an invalid name redefinition.<br>
> Diag(Loc, diag::err_redefinition_different_kind)<br>
> @@ -5494,15 +5517,9 @@<br>
> PrevNS = ND->getAnonymousNamespace();<br>
> }<br>
><br>
> - if (PrevNS && IsInline != PrevNS->isInline()) {<br>
> - // inline-ness must match<br>
> - Diag(Loc, diag::err_inline_namespace_mismatch)<br>
> - << IsInline;<br>
> - Diag(PrevNS->getLocation(), diag::note_previous_definition);<br>
> -<br>
> - // Recover by ignoring the new namespace's inline status.<br>
> - IsInline = PrevNS->isInline();<br>
> - }<br>
> + if (PrevNS && IsInline != PrevNS->isInline())<br>
> + DiagnoseNamespaceInlineMismatch(*this, NamespaceLoc, NamespaceLoc, II,<br>
> + &IsInline, PrevNS);<br>
> }<br>
><br>
> NamespaceDecl *Namespc = NamespaceDecl::Create(Context, CurContext, IsInline,<br>
><br>
> Modified: cfe/trunk/test/CXX/dcl.dcl/basic.namespace/namespace.def/p7.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/dcl.dcl/basic.namespace/namespace.def/p7.cpp?rev=165263&r1=165262&r2=165263&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/dcl.dcl/basic.namespace/namespace.def/p7.cpp?rev=165263&r1=165262&r2=165263&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/CXX/dcl.dcl/basic.namespace/namespace.def/p7.cpp (original)<br>
> +++ cfe/trunk/test/CXX/dcl.dcl/basic.namespace/namespace.def/p7.cpp Thu Oct 4 17:13:39 2012<br>
> @@ -3,11 +3,11 @@<br>
> namespace NIL {} // expected-note {{previous definition}}<br>
> inline namespace NIL {} // expected-error {{cannot be reopened as inline}}<br>
> inline namespace IL {} // expected-note {{previous definition}}<br>
> -namespace IL {} // expected-warning{{inline namespace cannot be re-opened as a non-inline namespace}}<br>
> +namespace IL {} // expected-warning{{inline namespace cannot be reopened as a non-inline namespace}}<br>
><br>
> namespace {} // expected-note {{previous definition}}<br>
> inline namespace {} // expected-error {{cannot be reopened as inline}}<br>
> namespace X {<br>
> inline namespace {} // expected-note {{previous definition}}<br>
> - namespace {} // expected-error {{cannot be reopened as non-inline}}<br>
> + namespace {} // expected-warning {{cannot be reopened as a non-inline namespace}}<br>
> }<br>
><br>
> Added: cfe/trunk/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp?rev=165263&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp?rev=165263&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp (added)<br>
> +++ cfe/trunk/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp Thu Oct 4 17:13:39 2012<br>
> @@ -0,0 +1,35 @@<br>
> +// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s<br>
> +<br>
> +// libstdc++ 4.6.x contains a bug where it defines std::__atomic[0,1,2] as a<br>
> +// non-inline namespace, then selects one of those namespaces and reopens it<br>
> +// as inline, as a strange way of providing something like a using-directive.<br>
> +// Clang has an egregious hack to work around the problem, by allowing a<br>
> +// namespace to be converted from non-inline to inline in this one specific<br>
> +// case.<br>
> +<br>
> +#ifdef BE_THE_HEADER<br>
> +<br>
> +#pragma clang system_header<br>
> +<br>
> +namespace std {<br>
> + namespace __atomic0 {<br>
> + typedef int foobar;<br>
> + }<br>
> + namespace __atomic1 {<br>
> + typedef void foobar;<br>
> + }<br>
> +<br>
> + inline namespace __atomic0 {}<br>
> +}<br>
> +<br>
> +#else<br>
> +<br>
> +#define BE_THE_HEADER<br>
> +#include "libstdcxx_atomic_ns_hack.cpp"<br>
> +<br>
> +std::foobar fb;<br>
> +<br>
> +using T = void; // expected-note {{here}}<br>
> +using T = std::foobar; // expected-error {{different types ('std::foobar' (aka 'int') vs 'void')}}<br>
> +<br>
> +#endif<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>
</div></div></blockquote></div><br>