search

cliqz-oss / keyvi

Keyvi - a key value index that powers Cliqz search engine. It is an in-memory FST-based data structure highly optimized for size and lookup performance.
https://cliqz.com
Apache License 2.0
179 stars 38 forks source link

Change type of row and collumn values to size_t #194

Closed lexplua closed 7 years ago

lexplua commented 7 years ago

Previously it was 'int' type in DistanceMatrix struct. It can cause overflows.

hendrikmuhs commented 7 years ago

LGTM,

Note: this part of the code is a bit dusty.

hendrikmuhs commented 7 years ago

but lets check the logs on CI (for warnings), after this is compiled.

(I do not get how exactly this can cause an overflow)

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 86.143% when pulling 398661cf4b75537d3c88635c59c1600ab955da5f on lexplua:master into e9d10955cc606389bf77278eca5a1d838ac8d65a on cliqz-oss:master.

hendrikmuhs commented 7 years ago

Logs look good.

@lexplua Thanks for your contribution. Can you do the const modifications? We will merge it then.

lexplua commented 7 years ago

@hendrikmuhs Yep, i will. I'd like to add test also.

hendrikmuhs commented 7 years ago

@lexplua

Do not be disappointed, your patch is very welcome! But sorry the unit test does not make sense in my opinion.

Can you remove the unit test from the PR? Feel free - if you like - to just remove the test but check in the file, actually we miss coverage for distance_matrix.h.

lexplua commented 7 years ago

@hendrikmuhs

Can you remove the unit test from the PR

  • Done !
coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 86.143% when pulling 20c27d8c48780c67aa5b14c151b3bbb348f2faae on lexplua:master into 4f9749765de9cae0098a13d925cf5f213be97511 on cliqz-oss:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 86.143% when pulling 20c27d8c48780c67aa5b14c151b3bbb348f2faae on lexplua:master into 4f9749765de9cae0098a13d925cf5f213be97511 on cliqz-oss:master.