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

Introduce -Wall flag, and fix compiler warnings #77

Merged
merged 15 commits into from May 4, 2018

Conversation

Projects
None yet
4 participants
@amit-cliqz
Collaborator

amit-cliqz commented Apr 25, 2018

Fixed compiler warnings and formatted those files which required fixes.

@coveralls

This comment has been minimized.

coveralls commented Apr 25, 2018

Coverage Status

Coverage increased (+0.02%) to 93.564% when pulling 9bb56a4 on amit-cliqz:warning into eed2304 on KeyviDev:master.

amit-cliqz added some commits Apr 25, 2018

@@ -173,7 +173,7 @@ class PrefixCompletion final {
if (data->traverser) {
TRACE("Current depth %d", exact_prefix + data->traverser.GetDepth() - 1);

int score =
size_t score =

This comment has been minimized.

@hendrikmuhs

hendrikmuhs Apr 25, 2018

Contributor

a score isn't a size type. What's the problem with int? I am open for replacing it with int32_t, to make it more explicit as int isn't clearly defined. A 64bit type would be to large and unnecessary, note that for the matrix calculation it would mean a massive size increase.

#include "dictionary/fsa/internal/memory_map_manager.h"
#include <boost/test/unit_test.hpp>
#include <cstring>

This comment has been minimized.

@hendrikmuhs

hendrikmuhs Apr 25, 2018

Contributor

clang-format does not get it right, the order should be:

  • C includes
  • empty line
  • C++ includes
  • empty line
  • boost includes
  • empty line
  • other 3rdparty includes
  • empty line
  • keyvi includes except for trace
  • empty line
  • trace include, with comment for enabling it

This comment has been minimized.

@amit-cliqz

amit-cliqz Apr 26, 2018

Collaborator

Thanks Hendrik for the information. Is it possible that clang-format behaves differently on different platform? I see one of the builds is failing if I correct the order. How should I proceed here if the clang-format doesn't accept the aforementioned order.

This comment has been minimized.

@hendrikmuhs

hendrikmuhs Apr 26, 2018

Contributor

it should not, but what matters is the clang-format version. We use 5 at the moment.

The trick with this is: clang-format sorts blocks of includes:

#include<b>
#include<c>
#include<d>
#include<a>

gets sorted to a,b,c,d but if you put an empty line between:

#include<b>
#include<c>

#include<d>
#include<a>

it sorts it to b,c and a,d

@@ -93,7 +93,7 @@ class FuzzyMatching final {
continue;
}

if (traverser_ptr_->IsFinalState() && metric_ptr_->GetScore() <= max_edit_distance_) {
if (traverser_ptr_->IsFinalState() && metric_ptr_->GetScore() <= static_cast<int>(max_edit_distance_)) {

This comment has been minimized.

@narekgharibyan

narekgharibyan May 2, 2018

Member

@amit-cliqz do we still need this type casting ?

This comment has been minimized.

@amit-cliqz

amit-cliqz May 2, 2018

Collaborator

@narekgharibyan yes, unless we refactor Distance matrix and GetScore function. I have a TODO for that.

This comment has been minimized.

@narekgharibyan

narekgharibyan May 2, 2018

Member

But looks like metric_ptr_->GetScore() returns size_t already and max_edit_distance_ has a type size_t as well.

https://github.com/KeyviDev/keyvi/pull/77/files#diff-7126fe200c21ac403b7f3de3cb69a0cdR181

This comment has been minimized.

@amit-cliqz

amit-cliqz May 2, 2018

Collaborator

Sorry, will remove the the type cast here.

@@ -173,6 +173,7 @@ class PrefixCompletion final {
if (data->traverser) {
TRACE("Current depth %d", exact_prefix + data->traverser.GetDepth() - 1);

// TODO(Amit) use int32_t instead of int.

This comment has been minimized.

@narekgharibyan

narekgharibyan May 2, 2018

Member

Can't we do the similar change to GetScore(), by just moving your TODO and type cast inside Put() method, and use size_t already here ?

boost::filesystem::create_directory(path);
MemoryMapManager m(chunkSize, path, "basic test");

m.GetAddress(0);
char* testptr = (char*) m.GetAddress(2 * chunkSize);
char* testptr = reinterpret_cast<char*>(m.GetAddress(2 * chunkSize));

This comment has been minimized.

@narekgharibyan

narekgharibyan May 2, 2018

Member

Maybe I'm mistaken, but do we need reinterpret_cast ? I assume static_cast should be enough.

This comment has been minimized.

@amit-cliqz

amit-cliqz May 3, 2018

Collaborator

Yes, static_cast should be enough here.

@hendrikmuhs

LGTM

@amit-cliqz amit-cliqz merged commit 61da211 into KeyviDev:master May 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 93.564%
Details

@amit-cliqz amit-cliqz deleted the amit-cliqz:warning branch May 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment