Merge pull request #82 from mapbox/oww

Expand 'mask' bitmask space from unsigned short to uint32_t (16=>32 bits)
mapbox · Jan 30, 2017 · 3694038 · 3694038
2 parents e43612a + 90e593e
commit 3694038
Show file tree

Hide file tree

Showing 3 changed files with 109 additions and 11 deletions.
diff --git a/package.json b/package.json
@@ -15,7 +15,7 @@
     "protozero": "~1.4.5"
   },
   "devDependencies": {
-    "tape": "3.0.x"
+    "tape": "^4.6.3"
   },
   "main": "./index.js",
   "scripts": {

diff --git a/src/binding.cpp b/src/binding.cpp
@@ -840,7 +840,7 @@ struct PhrasematchSubq {
     uint64_t phrase;
     unsigned short idx;
     unsigned short zoom;
-    unsigned short mask;
+    uint32_t mask;
 };
 
 struct Cover {
@@ -851,19 +851,19 @@ struct Cover {
     unsigned short y;
     unsigned short score;
     unsigned short idx;
-    unsigned short mask;
+    uint32_t mask;
     unsigned distance;
     double scoredist;
 };
 
 struct Context {
     std::vector<Cover> coverList;
-    unsigned mask;
+    uint32_t mask;
     double relev;
 
     Context(Context const& c) = default;
     Context(Cover && cov,
-            unsigned mask,
+            uint32_t mask,
             double relev)
      : coverList(),
        mask(mask),
@@ -877,7 +877,7 @@ struct Context {
         return *this;
     }
     Context(std::vector<Cover> && cl,
-            unsigned mask,
+            uint32_t mask,
             double relev)
      : coverList(std::move(cl)),
        mask(mask),
@@ -1187,8 +1187,7 @@ void coalesceSingle(uv_work_t* req) {
             added++;
 
             double relev = cover.relev;
-            // TODO correct default mask value?
-            unsigned mask = 0;
+            uint32_t mask = 0;
             contexts.emplace_back(std::move(cover),mask,relev);
         }
 
@@ -1312,7 +1311,7 @@ void coalesceMulti(uv_work_t* req) {
                 std::vector<Cover> covers;
                 covers.reserve(stackSize);
                 covers.push_back(cover);
-                unsigned context_mask = cover.mask;
+                uint32_t context_mask = cover.mask;
                 double context_relev = cover.relev;
 
                 for (unsigned a = 0; a < zCacheSize; a++) {
@@ -1323,7 +1322,7 @@ void coalesceMulti(uv_work_t* req) {
                         static_cast<uint64_t>(std::floor(cover.y/s));
                     pit = coalesced.find(pxy);
                     if (pit != coalesced.end()) {
-                        unsigned lastMask = 0;
+                        uint32_t lastMask = 0;
                         double lastRelev = 0.0;
                         for (auto const& parents : pit->second) {
                             for (auto const& parent : parents.coverList) {
@@ -1475,7 +1474,7 @@ NAN_METHOD(Cache::coalesce) {
 
             subq.weight = jsStack->Get(Nan::New("weight").ToLocalChecked())->NumberValue();
             subq.phrase = jsStack->Get(Nan::New("phrase").ToLocalChecked())->IntegerValue();
-            subq.mask = static_cast<unsigned short>(jsStack->Get(Nan::New("mask").ToLocalChecked())->IntegerValue());
+            subq.mask = static_cast<std::uint32_t>(jsStack->Get(Nan::New("mask").ToLocalChecked())->IntegerValue());
 
             // JS cache reference => cpp
             Local<Object> cache = Local<Object>::Cast(jsStack->Get(Nan::New("cache").ToLocalChecked()));

diff --git a/test/coalesce.test.js b/test/coalesce.test.js
@@ -690,3 +690,102 @@ test('coalesce args', function(assert) {
         });
     });
 })();
+
+// Mask overflow
+(function() {
+    var a = new Cache('a', 0);
+    var b = new Cache('b', 0);
+    var c = new Cache('c', 0);
+
+    var grids = [];
+    for (var i = 1; i < 10e3; i++) grids.push(Grid.encode({
+        id: i,
+        x: 0,
+        y: 0,
+        relev: 1,
+        score: 1
+    }));
+    a._set('grid', 0, 1, grids);
+
+    b._set('grid', 0, 1, [
+        Grid.encode({
+            id: 1,
+            x: 0,
+            y: 0,
+            relev: 1,
+            score: 1
+        })
+    ]);
+    c._set('grid', 0, 1, [
+        Grid.encode({
+            id: 1,
+            x: 0,
+            y: 0,
+            relev: 1,
+            score: 1
+        })
+    ]);
+
+    test('coalesceMulti mask safe', function(assert) {
+        assert.comment('start coalesce (mask: 2)');
+        coalesce([{
+            cache: a,
+            mask: 1 << 2,
+            idx: 0,
+            zoom: 0,
+            weight: 0.33,
+            phrase: 1
+        }, {
+            cache: b,
+            mask: 1 << 0,
+            idx: 1,
+            zoom: 0,
+            weight: 0.33,
+            phrase: 1
+        }, {
+            cache: c,
+            mask: 1 << 1,
+            idx: 2,
+            zoom: 0,
+            weight: 0.33,
+            phrase: 1
+        }], {}, function(err, res) {
+            assert.ifError(err);
+            assert.equal(res.length, 1, 'res length = 1');
+            assert.deepEqual(res[0].map(function(f) { return f.id; }), [1, 1, 1], '0.relev = 0.99');
+            assert.end();
+        });
+    });
+
+    test('coalesceMulti mask overflow', function(assert) {
+        assert.comment('start coalesce (mask: 18)');
+        coalesce([{
+            cache: a,
+            mask: 1 << 18,
+            idx: 0,
+            zoom: 0,
+            weight: 0.33,
+            phrase: 1
+        }, {
+            cache: b,
+            mask: 1 << 0,
+            idx: 1,
+            zoom: 0,
+            weight: 0.33,
+            phrase: 1
+        }, {
+            cache: c,
+            mask: 1 << 1,
+            idx: 2,
+            zoom: 0,
+            weight: 0.33,
+            phrase: 1
+        }], {}, function(err, res) {
+            assert.ifError(err);
+            assert.equal(res.length, 1, 'res length = 1');
+            assert.deepEqual(res[0].map(function(f) { return f.id; }), [1, 1, 1], '0.relev = 0.99');
+            assert.end();
+        });
+    });
+})();
+