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

Non-compliant (as far as I can tell) ValueMap allocator when using boehm #1288

Closed
copumpkin opened this issue Mar 23, 2017 · 6 comments · Fixed by #1289
Closed

Non-compliant (as far as I can tell) ValueMap allocator when using boehm #1288

copumpkin opened this issue Mar 23, 2017 · 6 comments · Fixed by #1289

Comments

@copumpkin
Copy link
Member

Right now, we have

typedef std::map<Symbol, Value *, std::less<Symbol>, gc_allocator<Value *> > ValueMap;

According to the official requirements, this is how it should look:

template<
    class Key,
    class T,
    class Compare = std::less<Key>,
    class Allocator = std::allocator<std::pair<const Key, T> >
> class map

Note that the allocator needs to be for a std::pair rather than just the value type. I think it should be a fairly simple fix, but it currently breaks with clang + libc++ 4, which are more strict about this sort of thing than they've been in the past.

cc @edolstra @shlevy @LnL7

@shlevy
Copy link
Member

shlevy commented Mar 23, 2017

Does it work if you do the gc_allocator for the std::pair instead?

@copumpkin
Copy link
Member Author

I haven't tried yet but I think @LnL7 is in a good position to try patching on the fly. Unfortunately it's behind a major LLVM+clang rebuild so it'll take me a while to get there 😄

@LnL7
Copy link
Member

LnL7 commented Mar 24, 2017

I tried with std::map<Symbol, Value *, std::less<Symbol>, gc_allocator<std::pair<const Symbol, Value *> > >, but that didn't seem to help. This is the build log https://gist.github.com/LnL7/26cc66bf15e2aeddb6729819ffa3aad4

@shlevy
Copy link
Member

shlevy commented Mar 24, 2017

Are you sure the change was picked up? That error clearly shows a gc_allocator<Value *> somewhere...

@LnL7
Copy link
Member

LnL7 commented Mar 24, 2017

You are right, it's in a different file for 1.11. Looks good. 😄

LnL7/nixpkgs@d179233

@shlevy
Copy link
Member

shlevy commented Mar 24, 2017

@LnL7 Can you submit a PR to 1.11-maintenance and master?

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

Successfully merging a pull request may close this issue.

3 participants