On 12/11/19 9:44 PM, Jonathan Wakely wrote:
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?
Indeed, here is the correct one.
diff --git a/libstdc++-v3/src/c++11/random.cc b/libstdc++-v3/src/c++11/random.cc
index 10fbe1dc4c4..04edc582b69 100644
--- a/libstdc++-v3/src/c++11/random.cc
+++ b/libstdc++-v3/src/c++11/random.cc
@@ -41,6 +41,7 @@
#include <cerrno>
#include <cstdio>
+#include <cctype> // For std::isdigit.
#if defined _GLIBCXX_HAVE_UNISTD_H && defined _GLIBCXX_HAVE_FCNTL_H
# include <unistd.h>
@@ -286,7 +287,7 @@ namespace std _GLIBCXX_VISIBILITY(default)
_M_mt.seed(seed);
#else
// Convert old default token "mt19937" or numeric seed tokens to "default".
- if (token == "mt19937" || isdigit((unsigned char)token[0]))
+ if (token == "mt19937" || std::isdigit((unsigned char)token[0]))
_M_init("default");
else
_M_init(token);
