Implement index-of expression
#1113
Conversation
SiarheiFedartsou
force-pushed
the
sf-index-of2
branch
from
353bbb6
to
b3f8046
Compare
SiarheiFedartsou
force-pushed
the
sf-index-of2
branch
from
b7a7074
to
cf14392
Compare
...id/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/style/expressions/ExpressionTest.kt
Show resolved
Hide resolved
SiarheiFedartsou
force-pushed
the
sf-index-of2
branch
from
653e6cd
to
9ae7021
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks for this great contribution!
Perhaps you could add a line to CHANGELOG.md and platform/android/CHANGELOG.md?
- Add support for [index-of expression](https://maplibre.org/maplibre-style-spec/expressions/#index-of) ([#1113](https://github.com/maplibre/maplibre-native/pull/1113))
Otherwise LGTM!
|
Thanks of contributing this @SiarheiFedartsou! |
SiarheiFedartsou
force-pushed
the
sf-index-of2
branch
from
0a6eb6e
to
cacd863
Compare
SiarheiFedartsou
force-pushed
the
sf-index-of2
branch
from
cacd863
to
b9feda8
Compare
Done 👍 |
for more information, see https://pre-commit.ci
| EvaluationResult IndexOf::evaluateForArrayInput(const std::vector<Value> &array, | ||
| const Value &keywordValue, | ||
| size_t fromIndexValue) const { | ||
| for (size_t index = fromIndexValue; index < array.size(); ++index) { |
There was a problem hiding this comment.
Doesn't this allow out-of-bounds access (~crashing) with ["index-of", "foo", "foobar", -100000]?
There was a problem hiding this comment.
Ah nevermind, it's size_t, which is unsigned.
There was a problem hiding this comment.
This is still different from the JS version probably:
// Should be ["index-of", "foo", "__foo__foo", -100]
"__foo__foo".indexOf("foo", -100) // returns 2
// Should be ["index-of", "foo", ["foo", "foo"], -100]
["foo", "foo"].indexOf("foo", -100) // returns 0I believe the native version would return -1 in both of these cases.
There was a problem hiding this comment.
Thanks for your comments @JannikGM !
I.e. JS version just makes fromIndex = std::max(fromIndex, 0) ? Okay, will apply a fix... Thanks.
There was a problem hiding this comment.
Yes 👍
Background
I didn't try it in the maplibre-gl-js version. But I did skim over maplibre-gl-js code for index-of.
The code (in my previous comment) are browser devtools results. I believe the maplibre expressions mentioned in the comment would result in similar code running in maplibre-gl-js (haystack.indexOf(needle, fromIndex), where fromIndex can be negative).
So in maplibre-gl-js there isn't an explicit Math.max(fromIndex, 0), but <...>.indexOf seems to do it implicitly internally (negative fromIndex behaves as if the index was 0 in <string>.indexOf and <array>.indexOf).
So in the C++ version we should do the explicit std::max before casting to unsigned integers.
| return EvaluationError{"Expected third argument to be of type number, but found " + | ||
| toString(typeOf(*evaluatedFromIndex)) + " instead."}; | ||
| } | ||
| fromIndexValue = static_cast<size_t>(evaluatedFromIndex->get<double>()); |
There was a problem hiding this comment.
This should have been something like fromIndexValue = static_cast<size_t>(std::max(evaluatedFromIndex->get<double>(), 0.0)); to avoid undefined behaviour of the static_cast for negative inputs.
See my other comment: #1113 (comment).
(There's still undefined behaviour for large numbers, but it's probably not a relevant case as it implies a huge haystack or very bad math error; negative indices on the other hand can easily happen and might be useful)
Related to #264