Skip to content

Fix: kh_hash_equal was not instantiating for char* keys#4

Closed
n8sh wants to merge 1 commit intoblachlylab:masterfrom
n8sh:fix-cstring-keys
Closed

Fix: kh_hash_equal was not instantiating for char* keys#4
n8sh wants to merge 1 commit intoblachlylab:masterfrom
n8sh:fix-cstring-keys

Conversation

@n8sh
Copy link
Contributor

@n8sh n8sh commented Aug 26, 2020

No description provided.

@jblachly
Copy link
Member

Thank you @n8sh -- will be happy to accept this PR if you can explain to me why it works =)

In my D code generally I have purposefully avoided qualifying function parameters with in due to the admonition , which may be outdated, in the docs:

defined as scope const. However in has not yet been properly implemented so it's current implementation is equivalent to const. It is recommended to avoid using in until it is properly defined and implemented. Use scope const or const explicitly instead.

const also fixes this issue, although I am not clear how that helps the template parameter deduction in the case of char *

Would adding a template constraint isPointerTarget also be a reasonble way to resolve the char * issue?

https://dlang.org/phobos/std_traits.html#.PointerTarget

Thanks again for your help -- will look at your other PR later today

@n8sh
Copy link
Contributor Author

n8sh commented Aug 27, 2020

Thank you @n8sh -- will be happy to accept this PR if you can explain to me why it works =)

Harder than explaining why it works would be to explain why the template doesn't work without it. Even if the compiler wasn't trying to do anything clever it seems like foo(T)(T* a, T* b) if called with a first argument of type const char* and a second argument of type char* would naturally infer T to be const char, but it doesn't. All I can say is telling the compiler to treat both arguments as const gives it enough help to let it do the rest.

Would adding a template constraint isPointerTarget also be a reasonble way to resolve the char * issue?

Yes that would work too, it's just a matter of preference.

@jblachly
Copy link
Member

@n8sh OK I've got it. In this case the compiler is treating in as const , which based on everything I can tell we should still be avoiding ... https://forum.dlang.org/thread/ewihuwtljnetweunhuhy@forum.dlang.org

qualifying the function parameters as const also works well as you say, and this is what gave me the clue.

Function call at line 260 takes const(kh_t)* h (equivalent to const this) and the const, being transitive, applies const to h.keys[] which is the first parameter to the kh_hash_equals template. Template deduction fails because no matching template has const param.

@jblachly
Copy link
Member

The semantics of in may be changing in the future: dlang/dmd#11000 such that it is no longer simply lowered to const, thus I would prefer to go with scope const explicitly in this template. If you don't have a chance to update the PR I will try to do so sometime by the week-end.

@jblachly jblachly closed this in f8d4efe Aug 30, 2020
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 this pull request may close these issues.

2 participants