Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix regression, empty dict #120

Merged
merged 3 commits into from
Sep 7, 2016
Merged

Conversation

hendrikmuhs
Copy link
Contributor

This fixes a regression, creation of an empty dict segfaults. Also added missing test code for this.

@narek-cliqz

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 84.801% when pulling a29eb34 on hendrik-cliqz:fix-empty-dict into e9d2b90 on cliqz-oss:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 84.801% when pulling aa47e0e on hendrik-cliqz:fix-empty-dict into e9d2b90 on cliqz-oss:master.

@@ -160,14 +160,14 @@ class DictionaryCompiler
value_store_->CloseFeeding();
sorter_.end();
sorter_.merge_runs();
CreateGenerator();

// check that at least 1 item is there
if (!sorter_.can_pull()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if we remove this check and add if ( sorter_.can_pull()) on line: 170 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

@ghost
Copy link

ghost commented Sep 7, 2016

LGTM

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.796% when pulling 31ec7e8 on hendrik-cliqz:fix-empty-dict into e9d2b90 on cliqz-oss:master.

@hendrikmuhs hendrikmuhs merged commit 7f7d678 into cliqz-oss:master Sep 7, 2016
hendrikmuhs added a commit to hendrikmuhs/keyvi that referenced this pull request Jan 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants