Repository navigation
indexOf empty string in buffer should return 0 #13023
Description
Activity
Hmm - the Buffer#indexOf documentation explicitly states that it will return -1 if the value is not found. https://nodejs.org/api/buffer.html#buffer_buf_indexof_value_byteoffset_encoding
Returns: The index of the first occurrence of value in buf or -1 if buf does not contain value
I believe this is expected behavior.
See what @addaleax said below!
The following information is wrong, see #13023 (comment)!
Bufferfunctions more akin to an array than a string. (In fact it extends theUint8Arrayclass!)["a", "b", "c"].indexOf("")returns -1 also.@lance I think this is a bug – it makes sense for an empty input value to say that it is found immediately, without any searching. Also,
String.prototype.indexOfbehaves the same way, and I think consistency is important (especially when there’s no reason to deviate) because developers expect them to behave the same way.@TimothyGu Unlike
Array#indexOf,Buffer#indexOfdoesn’t compare elements – it compares subsequences, and there’s no equivalent of an empty subsequence for arrays. :)This should be enough to fix:
diff
diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 367af6592ff3..32a942a42c4e 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -974,7 +974,13 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) { const size_t needle_length = StringBytes::Size(args.GetIsolate(), needle, enc); - if (needle_length == 0 || haystack_length == 0) { + if (needle_length == 0) { + const double first_index = is_forward ? 0 : haystack_length; + args.GetReturnValue().Set(first_index); + return; + } + + if (haystack_length == 0) { return args.GetReturnValue().Set(-1); } @@ -1077,7 +1083,13 @@ void IndexOfBuffer(const FunctionCallbackInfo<Value>& args) { const char* needle = buf_data; const size_t needle_length = buf_length; - if (needle_length == 0 || haystack_length == 0) { + if (needle_length == 0) { + const double first_index = is_forward ? 0 : haystack_length; + args.GetReturnValue().Set(first_index); + return; + } + + if (haystack_length == 0) { return args.GetReturnValue().Set(-1); }
Welcome for the changes. But I would like to explain consistency more.
In doc:Bufferclass implements the Uint8Array API in a manner that is more optimized and suitable for Node.js' use casesThere is no consistency in the first place.
Buffer.from("abc").indexOf("")= -1, is more consistent with the Uint8Array API.However, existing
Buffer#indexOfimplements theString#indexOfmore likely. Especially whenBuffer.from("abc").indexOf("bc")= 1. Then,Buffer.from("abc").indexOf("")should be0.Hmm, since the spec has already defined
TypedArray.prototype.indexOf()as:22.2.3.14%TypedArray%.prototype.indexOf ( searchElement [ , fromIndex ] )
%TypedArray%.prototype.indexOf is a distinct function that implements the same algorithm as Array.prototype.indexOf as defined in 22.1.3.12 except that the this object's [[ArrayLength]] internal slot is accessed in place of performing a [[Get]] of "length"I would say the current behavior is more consistent..especially when we are on our way making APIs that accept
Buffers also acceptUint8Arrays@joyeecheung That’s what
Uint8Array.prototype.indexOfdoes, but as mentioned above we already deviate very strongly from that – we accept sequences instead of individual values as input, and there’s no way of mapping that toUint8Array.prototype.indexOf’s type signature.- added a commit that references this issue
on May 18, 2017 - added a commit that references this issue
on May 19, 2017
"abc".indexOf("")return 0.But
Buffer.from("abc").indexOf("")return -1.Expected result : 0