Re: [PATCH] Fix gnu-versioned-namespace build

[Jonathan Wakely <jwakely@redhat.com>]

On 11/12/19 21:23 +0100, François Dumont wrote:
>On 12/11/19 12:16 PM, Jonathan Wakely wrote:
>>On 11/12/19 08:29 +0100, François Dumont wrote:
>>>I plan to commit this tomorrow.
>>>
>>>Note that rather than just adding the missing 
>>>_GLIBCXX_[BEGIN,END]_VERSION_NAMESPACE I also move anonymous 
>>>namespace usage outside std namespace. Let me know if it was 
>>>intentional.
>>
>>It was intentional, why move it?
>
>I just don't get the intention so I proposed to move it. But there are 
>indeed other usages of this pattern in other src files.
>
>Note that in src/c++11/debug.cc I am using anonymous namespace at 
>global scope, is that wrong ?

It's certainly more fragile, so arguably it's wrong, yes.

Consider:

// some libc function in a system header we don't control:
extern "C" void __foo();

// libstdc++ code in a .cc file:
namespace
{
  void foo() { }
}
namespace std
{
  void bar() { foo(); }
}

This fails to compile, because the name foo is ambiguous in the global
scope. We don't control the libc headers, so we don't know all the
names they might declare at global scope.

If you don't put the unnamed namespace at global scope, the problem
simply doesn't exist:

namespace std
{
  namespace
  {
    void foo() { }
  }

  void bar() { foo(); }
}

Now it doesn't matter what names libc puts in the global scope,
because we're never looking for foo in the global scope.

It's obviously better to add our declarations to our own namespace
that we control, not to the global namespace (and an unnamed namespace
at global scope effectively adds the names to the global namespace).

>>
>>Adding the BEGIN/END_VERSION macros is unnecessary. Those namespaces
>>are inline, so std::random_device already refers to
>>std::__8::random_device when the original declaration was in the
>>versioned namespace.
>
>Ok. I must confess I wasn't clear about this but looking at other src 
>files, at least in src/c++11, was showing that it is done almost 
>always this way, I guess we could cleanup those files.
>
>>
>>The only fix needed here seems to be qualifying std::isdigit (and
>>strictly-speaking we should also include <cctype> to declare that).
>>
>Like in attached patch ?

Did you attach the wrong patch?