Add non-const char* overload to LookupByKey#8848
Add non-const char* overload to LookupByKey#8848jtdavis777 wants to merge 8 commits intogoogle:masterfrom
Conversation
|
the "better" way to fix this might be to SFINAE the |
|
Yes, this doesn't seem a great solution since the problem is the overly generic template version that will just take any type. Given that the users of these functions are mostly generated code, I wonder if we can't simply improve how this is called in generated code instead. Do they even need to be overloaded? What string types can this possibly be called with to warrant the overly generic template? How does the original reporter call this to cause an error? Are they calling this function directly? |
| int KeyCompareWithValue(const char *_key) const { | ||
| return strcmp(key()->c_str(), _key); | ||
| } | ||
| int KeyCompareWithValue(char *_key) const { |
There was a problem hiding this comment.
Why can we not just make this the only version? I believe const char * passed into a char * will work.
There was a problem hiding this comment.
Thats a great suggestion. Will implement.
There was a problem hiding this comment.
const char * passed to char * generally won't work?
There was a problem hiding this comment.
Sorry, I meant to write it the other way around.
There was a problem hiding this comment.
I knew what you meant :) it just depends on if the argument match considers it close enough.
There was a problem hiding this comment.
oh, no, this is the root of the issue. the original version is a const char* only -- the issue is that a const char* maps onto the templated version template<typename StringType> int KeyCompareWithValue(const StringType&)
So the two options are either
- providing an explicit overload for
char*, or - adding SFINAE conditions to the template deduction (which is a bit gross in C++11)
dbaileychess
left a comment
There was a problem hiding this comment.
Also add a unit test for this. Find out where we do a call to KeyCompareWithValue in one of our tests and pass in both a const char* and char*.
|
test added. |
Fixes #8557