Do not cast a potentially non-String value to String, use ToString() instead.

[iamstolis]

Note that Reader::createString() may not return String, it may return a Buffer (when return_buffers is set to true). So, the variable 'v' may not hold String. Moreover, 'v.As()' doesn't convert the value to String, it works more like a cast. Of course, it is incorrect to cast Buffer to String. Unfortunately, the non-debug version of Node.js has various checks turned off. So, you cannot see this issue normally, but when you execute tests on the debug version of Node.js, i.e., run node_g test/reader.js then you can see the following crash caused by the incorrect cast of 'v'.

#
# Fatal error in ../deps/v8/src/api.h, line 400
# Check failed: that == __null || (*reinterpret_cast<v8::internal::Object* const*>(that))->IsString().
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::Utils::OpenHandle(v8::String const*, bool)
 3: v8::Exception::Error(v8::Local<v8::String>)
 4: 0x7efd7e262cb5
 5: redisReaderGetReply
 6: hiredis::Reader::Get(Nan::FunctionCallbackInfo<v8::Value> const&)
 7: 0x7efd7e2626c6
 8: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&))
 9: 0xdeaebe
10: 0xde59ae
11: 0xde5919
12: 0x1a1e1b8060bb
Illegal instruction

[badboy]

Casting sounds wrong indeed. Is there a way to see this problem happening with a normal node build?

[iamstolis]

I am sorry, I don't know how to reproduce this issue using a non-debug build. I tried to produce some test-case but I was not successful. On the other hand, I hope that you understand that this issue is worth fixing even without such a test-case.

[badboy]

I understand. I will check the API docu tomorrow just to make sure, but will this pull in then.

[iamstolis]

Did you find any problem while checking the API docs? Is there anything I can help with?

[badboy]

@not-a-robot r+

[ghost]

:pushpin: Commit 360c948 has been approved by badboy

[ghost]

:zap: Test exempted - status