Bug 28514 - [4.2 Regression] libstdc++ vs. anonymous namespaces
Summary: [4.2 Regression] libstdc++ vs. anonymous namespaces
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.2.0
: P3 normal
Target Milestone: 4.2.0
Assignee: Jason Merrill
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-27 17:01 UTC by Benjamin Kosnik
Modified: 2006-10-17 11:52 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
reproducer with today's gcc (211.35 KB, application/octet-stream)
2006-07-27 17:04 UTC, Benjamin Kosnik
Details
work-in-progress patch to convert libstdc++ to anonymous namespaces (33.80 KB, patch)
2006-07-27 22:33 UTC, Benjamin Kosnik
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Kosnik 2006-07-27 17:01:58 UTC
Am running into this kind of thing when I convert include/ext/rope and friends to using anonymous namespace.

This simplified source doesn't show the bug, sadly, but I'll put up pre-processed code that demonstrates it in a bit.

namespace __gnu_cxx
{
  namespace 
  {
    enum _Tag1 {_S_leaf1, _S_concat1, _S_substringfn1, _S_function1};
  }

  namespace constants
  {
    enum _Tag2 {_S_leaf2, _S_concat2, _S_substringfn2, _S_function2};
  }

  template<typename T>
  struct A
  {
    static void foo(const T i)
    {    
      int j;
      switch(i)
	{
	case _S_concat1: // error here, invalid integer constant
	  j = 5;
	  break;
	case constants::_S_leaf2:
	  j = 5;
	  break;
	default:
	  j = 0;
	}
    }
  };

Code seems correct, but something about PCH kills it.
Comment 1 Benjamin Kosnik 2006-07-27 17:04:04 UTC
Created attachment 11957 [details]
reproducer with today's gcc


To reproduce, do:

/mnt/share/bld/gcc/./gcc/xgcc -shared-libgcc -B/mnt/share/bld/gcc/./gcc -nostdinc++ -Winvalid-pch -Wno-deprecated -x c++-header i686-pc-linux-gnu/bits/extc++.h.gch.ii -o i686-pc-linux-gnu/bits/extc++.h.gch/02g.gch

Or equivalent, based on your current build compiler directory locations.

Get:

/mnt/share/bld/gcc/i686-pc-linux-gnu/libstdc++-v3/include/ext/ropeimpl.h: In static member function 'static _CharT __gnu_cxx::rope<_CharT, _Alloc>::_S_fetch(__gnu_cxx::_Rope_RopeRep<_CharT, _Alloc>*, size_t) [with _CharT = char, _Alloc = std::allocator<char>]':
/mnt/share/bld/gcc/i686-pc-linux-gnu/libstdc++-v3/include/ext/rope:1980:   instantiated from '_CharT __gnu_cxx::rope<_CharT, _Alloc>::operator[](size_t) const [with _CharT = char, _Alloc = std::allocator<char>]'
/mnt/share/bld/gcc/i686-pc-linux-gnu/libstdc++-v3/include/ext/rope:2894:   instantiated from here
/mnt/share/bld/gcc/i686-pc-linux-gnu/libstdc++-v3/include/ext/ropeimpl.h:1334: error: case label does not reduce to an integer constant
Comment 2 Benjamin Kosnik 2006-07-27 17:05:18 UTC
Jason can you look at this plz?
Comment 3 Benjamin Kosnik 2006-07-27 22:32:36 UTC
Changing just the first

	    case _S_concat:

to

	    case ::_S_concat:

Fixes this. Wierd.

I noticed a couple of other random lookup issues, or non-issues that surprised me. One was with static functions in global-scope anonymous namespaces have to be explicitly qualified with a :: operator. (mt_allocator.cc patches)

global-scope enums used as integer constants in case labels of switch statements. Fully qualified too?

Anyway. I'll attach the work-in-progress patch to convert libstdc++ __gnu_internal namespace to anonymous namespaces.

-benjamin
Comment 4 Benjamin Kosnik 2006-07-27 22:33:18 UTC
Created attachment 11961 [details]
work-in-progress patch to convert libstdc++ to anonymous namespaces
Comment 5 Benjamin Kosnik 2006-07-27 22:33:45 UTC
change title
Comment 6 Andrew Pinski 2006-07-29 05:25:49 UTC
I think this is 4.2 regression now but I need to reduce it.
Comment 7 Jason Merrill 2006-07-31 08:16:28 UTC
The testcase gives the same errors for me when compiled as normal C++ as in PCH mode.

The problem seems to be that you're removing the Rope_constants namespace name and creating a name lookup collision between the _S_concat enumerator and the _S_concat function in rope.

Why would you want to mess with Rope_constants, anyway?  It doesn't have any symbols in it, it just controls name lookup.  When you take it away, name lookup changes, and things blow up.  Moving them to the anonymous namespace has the same effect for name lookup as declaring them directly in __gnu_cxx.

Furthermore, defining _Tag in an anonymous namespace will cause the compiler to give all functions with _Tag in their signature internal linkage.  I don't understand why you would want this.
Comment 8 Jason Merrill 2006-07-31 08:20:12 UTC
In general, I think using the anonymous namespace in headers is not what you want.
Comment 9 Benjamin Kosnik 2006-09-04 15:22:27 UTC
> Furthermore, defining _Tag in an anonymous namespace will cause the compiler to
> give all functions with _Tag in their signature internal linkage.  I don't
> understand why you would want this.

This is precisely one reason why anonymous namespaces are useful. It provides a very viceral way to sanity check an API. Make sure that the private parts really are private, say.

Please, let's leave <rope> out of it.

I think that there are good reasons to use anonymous namespaces in headers, even if you disagree with these designs personally.


-benjamin
Comment 10 Jason Merrill 2006-09-07 00:14:06 UTC
Subject: Re:  [4.2 Regression] libstdc++ vs. anonymous namespaces

bkoz at gcc dot gnu dot org wrote:
> This is precisely one reason why anonymous namespaces are useful. It provides a
> very viceral way to sanity check an API. Make sure that the private parts
> really are private, say.

Yes, but there's a difference between private and internal.  This is 
especially problematic for templates; if you give template 
instantiations internal linkage, we can't share them between translation 
units anymore and you get code bloat.  Do you really want a copy of the 
list of primes from the hashtable policy code in each translation unit 
that uses it?

It seems to me that Rope_constants, _Private in <tr1/random> and the 
hashtable policy stuff were in special namespaces just to avoid name 
lookup pollution.  If you really want them also to be unique to each 
translation unit you could insert an anonymous namespace inside the 
preexisting namespace and not have to mess with name lookup at all.

I thought after my earlier comments you would put Rope_constants back, 
but now that I actually look at what you checked in I see that you just 
added explicit global scope to the uses.  That kind of cluttering up of 
the global namespace seems like a mistake to me; _Tag isn't a very 
unique name.  Changing _Private to an anonymous namespace has the same 
problem, except it's only cluttering up tr1 instead of the global 
namespace.  In both cases inserting an anonymous namespace inside the 
named namespace seems both better and less work.

> Please, let's leave <rope> out of it.

You reported a problem compiling <rope>, it's hard to respond without 
talking about <rope>.

> I think that there are good reasons to use anonymous namespaces in headers,
> even if you disagree with these designs personally.

Please elaborate.  Why do you want _Tag, _Select, X::primes, etc. to be 
unique to each translation unit?

Jason
Comment 11 Benjamin Kosnik 2006-10-17 11:48:48 UTC
OK. I've reverted these anonymous namespace conversions.

Namespace that are just trying to squester name lookup should be spelled as nested "detail" namespaces. Namespaces that are trying to prohibit exports via internal linkage should be anonymous namespaces. 

I believe this usage is now uniform.
Comment 12 Benjamin Kosnik 2006-10-17 11:52:35 UTC
Should be libstdc++ bug.
Comment 13 Benjamin Kosnik 2006-10-17 11:56:33 UTC
Subject: Bug 28514

Author: bkoz
Date: Tue Oct 17 11:56:21 2006
New Revision: 117824

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=117824
Log:
2006-10-17  Benjamin Kosnik  <bkoz@redhat.com>

	PR libstdc++/28514 
	* include/bits/cpp_type_traits.h (__detail): Uglify namespace.
	* include/ext/rope: Remove global-scope anonymous namespace, use
	nested __detail. Fixup resulting formatting issues.
	* include/ext/ropeimpl.h: Same.
	* include/tr1/hashtable_policy.h: Remove anonymous namespace
	nesting for __detail.
	* include/tr1/random: Revert anonymous namespace to nested
	__detail namespace.
	* include/tr1/random.tcc: Same.
	* src/ext-inst.cc: Fixups for above.


Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/cpp_type_traits.h
    trunk/libstdc++-v3/include/ext/rope
    trunk/libstdc++-v3/include/ext/ropeimpl.h
    trunk/libstdc++-v3/include/tr1/hashtable_policy.h
    trunk/libstdc++-v3/include/tr1/random
    trunk/libstdc++-v3/include/tr1/random.tcc
    trunk/libstdc++-v3/src/ext-inst.cc