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