Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String.h specialize hash<string> causes problems for libc++. #74

Closed
likan999 opened this issue Jul 8, 2014 · 3 comments
Closed

String.h specialize hash<string> causes problems for libc++. #74

likan999 opened this issue Jul 8, 2014 · 3 comments

Comments

@likan999
Copy link

likan999 commented Jul 8, 2014

The following simple program will produce error under libc++:

#include <cassert>
#include <string>
#include <unordered_map>

using namespace std;

namespace std {
template <class C>
struct hash<std::basic_string<C> > : private hash<const C*> {
  size_t operator()(const std::basic_string<C> & s) const {
    return hash<const C*>::operator()(s.c_str());
  }
};
}

const static unordered_map<string, int> m = {
  {"abc", 1},
  {"xyz", 2},
};

int main() {
  assert(m.find("abc") != m.end());
}

String.h shouldn't specialize hash function for libc++.

@tudor
Copy link
Contributor

tudor commented Jul 8, 2014

Can you please paste this as text somehow? It seems HTML ate a lot of your
angle brackets :)

Also, what is the error that you're getting? Is it that hashstd::string
is already defined?

On Mon, Jul 7, 2014 at 8:02 PM, likan999 notifications@github.com wrote:

The following simple program will produce error under libc++:

#include
#include
#include

using namespace std;

namespace std {
template
struct hashstd::basic_string : private hash {
size_t operator()(const std::basic_string & s) const {
return hash::operator()(s.c_str());
}
};
}

const static unordered_map m = {
{"abc", 1},
{"xyz", 2},
};

int main() {
assert(m.find("abc") != m.end());
}

String.h shouldn't specialize hash function for libc++.


Reply to this email directly or view it on GitHub
#74.

@likan999
Copy link
Author

likan999 commented Jul 8, 2014

@tudor, the specialization is copied from String.h to demonstrate the problem. Simply including "String.h" can also reproduce the same issue. The error is the code successfully compiles under libc++ but at runtime the assertion fails. I guess the culprit is libc++ has a different hash implementation.

I guess the specialization was introduced when c++11 was not yet fully implemented by mainline compilers and hash was not available. But since hash is in C++11 standard, and major compilers supports c++11 faily well, it is no longer needed?

@tudor
Copy link
Contributor

tudor commented Jul 8, 2014

You're right. Our specialization is actually broken! (because we make
hash call hash<const char*> on s.c_str(), which is clearly bad, as
hash<const char*> doesn't hash the strings, only the pointer value)

We haven't seen this at FB because libstdc++ provides full specializations
(as required by the standard), so hash is more specific than
template hash<basic_string>, so the libstdc++ specialization
wins.

Clearly libc++ does something different (and it's not fully standard
compliant), but, regardless, these specializations are unnecessary AND
broken, so I'll fix.

On Tue, Jul 8, 2014 at 9:51 AM, likan999 notifications@github.com wrote:

@tudor https://github.com/tudor, the specialization is copied from
String.h to demonstrate the problem. By simply including "String.h" can
also reproduce the same issue. The error is the code successfully compiles
under libc++ but at runtime the assertion fails. I guess the culprit is
libc++ has a different hash implementation.

I guess the specialization was introduced when c++11 was not yet fully
implemented by mainline compilers and hash was not available. But since
hash is in C++11 standard, and major compilers supports c++11 faily well,
it is no longer needed?


Reply to this email directly or view it on GitHub
#74 (comment).

@tudor tudor closed this as completed Jul 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants