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

remove zero-byte restriction in keyvi, to allow search of arbitrary binary data #104

Closed

Conversation

hendrikmuhs
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 84.047% when pulling be9ae44 on hendrik-cliqz:remove-c-string-limitation into 5215168 on cliqz-oss:master.

@coveralls
Copy link

coveralls commented Jul 11, 2016

Coverage Status

Coverage increased (+0.1%) to 84.162% when pulling e7c10db on hendrik-cliqz:remove-c-string-limitation into 5215168 on cliqz-oss:master.

@hendrikmuhs hendrikmuhs changed the title remove const char* in favor of std::string remove zero-byte restriction in keyvi, to allow the search of arbitrary binary data Jul 11, 2016
uint64_t state = fsa_->GetStartState();
size_t key_length = strlen(key);
size_t key_length = key.size();
Copy link

Choose a reason for hiding this comment

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

In general I prefer and have a habit to define variables which are not going to be changed as const. This improves readability, prevents from some errors and even sometimes can help compiler to generate more efficient code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 changing at least the lines I touched

@hendrikmuhs hendrikmuhs changed the title remove zero-byte restriction in keyvi, to allow the search of arbitrary binary data remove zero-byte restriction in keyvi, to allow search of arbitrary binary data Jul 11, 2016
uint64_t state = fsa_->GetStartState();
size_t text_length = strlen(key);
size_t text_length = key.size();
Copy link

Choose a reason for hiding this comment

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

see above.

@@ -220,7 +218,7 @@ final {
stack_->UpdateWeights(0, input_key.size() + 1, weight);
}

last_key_ = key;
last_key_ = std::move(input_key);
Copy link

Choose a reason for hiding this comment

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

input_key is a const, so std::move call will return const std::string&& which is not the same as std::string&& and in the end anyway copy-constructor will be called as it's a better match, not move

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 84.166% when pulling d7a6dfe on hendrik-cliqz:remove-c-string-limitation into 5215168 on cliqz-oss:master.

@coveralls
Copy link

coveralls commented Jul 14, 2016

Coverage Status

Coverage increased (+0.8%) to 84.9% when pulling 44f7a0a on hendrik-cliqz:remove-c-string-limitation into 5215168 on cliqz-oss:master.

@coveralls
Copy link

coveralls commented Jul 15, 2016

Coverage Status

Coverage increased (+0.8%) to 84.82% when pulling bdad7d3 on hendrik-cliqz:remove-c-string-limitation into 5215168 on cliqz-oss:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 84.923% when pulling bb89f13 on hendrik-cliqz:remove-c-string-limitation into c8db8e6 on cliqz-oss:master.

@hendrikmuhs
Copy link
Contributor Author

rebased in #115

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