From 18d67772fdac7e32a46dfbc6538b07ec2de61b05 Mon Sep 17 00:00:00 2001 From: Cai Yudong Date: Tue, 11 Aug 2020 22:16:42 +0800 Subject: [PATCH] enable -Werror to improve code quality (#3221) * clean warning Signed-off-by: yudong.cai * update faiss make options Signed-off-by: yudong.cai * update changelog Signed-off-by: yudong.cai * fix clang format Signed-off-by: yudong.cai * remove wrong file Signed-off-by: yudong.cai * update unittest Signed-off-by: yudong.cai Co-authored-by: shengjun.li --- CHANGELOG.md | 1 + core/CMakeLists.txt | 2 +- .../src/index/cmake/ThirdPartyPackagesCore.cmake | 5 +++-- .../knowhere/index/vector_index/IndexHNSW.cpp | 8 ++++++-- .../knowhere/index/vector_index/IndexRHNSW.cpp | 6 +++--- .../index/vector_index/impl/nsg/Distance.h | 1 + .../knowhere/index/vector_index/impl/nsg/NSG.cpp | 4 ++-- .../thirdparty/faiss/utils/instruction_set.h | 4 ++-- core/src/index/thirdparty/hnswlib/hnswalg.h | 2 +- .../unittest/test_structured_index_sort.cpp | 16 ++++++++-------- core/thirdparty/dablooms/dablooms.cpp | 1 - 11 files changed, 28 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cfc2f87..13e1a182 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ Please mark all changes in change log and use the issue from GitHub - \#2841 Replace IndexType/EngineType/MetricType - \#2858 Unify index name in db - \#2884 Using BlockingQueue in JobMgr +- \#3220 Enable -Werror to improve code quality ## Task diff --git a/core/CMakeLists.txt b/core/CMakeLists.txt index c24eea0c..96bc68dc 100644 --- a/core/CMakeLists.txt +++ b/core/CMakeLists.txt @@ -100,7 +100,7 @@ append_flags( CMAKE_CXX_FLAGS "-fPIC" "-DELPP_THREAD_SAFE" "-fopenmp" - "-Werror=return-type" + "-Werror" ) # **************************** Source files **************************** diff --git a/core/src/index/cmake/ThirdPartyPackagesCore.cmake b/core/src/index/cmake/ThirdPartyPackagesCore.cmake index fc5aabb8..66957970 100644 --- a/core/src/index/cmake/ThirdPartyPackagesCore.cmake +++ b/core/src/index/cmake/ThirdPartyPackagesCore.cmake @@ -119,8 +119,9 @@ set(THIRDPARTY_DIR "${INDEX_SOURCE_DIR}/thirdparty") string(TOUPPER ${CMAKE_BUILD_TYPE} UPPERCASE_BUILD_TYPE) -set(EP_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}}") -set(EP_C_FLAGS "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${UPPERCASE_BUILD_TYPE}}") +set(FAISS_FLAGS "-DELPP_THREAD_SAFE -fopenmp -Werror=return-type") +set(EP_CXX_FLAGS "${FAISS_FLAGS} ${CMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}}") +set(EP_C_FLAGS "${FAISS_FLAGS} ${CMAKE_C_FLAGS_${UPPERCASE_BUILD_TYPE}}") if (NOT MSVC) # Set -fPIC on all external projects diff --git a/core/src/index/knowhere/knowhere/index/vector_index/IndexHNSW.cpp b/core/src/index/knowhere/knowhere/index/vector_index/IndexHNSW.cpp index 422b7ee2..1ceae454 100644 --- a/core/src/index/knowhere/knowhere/index/vector_index/IndexHNSW.cpp +++ b/core/src/index/knowhere/knowhere/index/vector_index/IndexHNSW.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -82,11 +83,14 @@ IndexHNSW::Train(const DatasetPtr& dataset_ptr, const Config& config) { int64_t rows = dataset_ptr->Get(meta::ROWS); hnswlib::SpaceInterface* space; - if (config[Metric::TYPE] == Metric::L2) { + std::string metric_type = config[Metric::TYPE]; + if (metric_type == Metric::L2) { space = new hnswlib::L2Space(dim); - } else if (config[Metric::TYPE] == Metric::IP) { + } else if (metric_type == Metric::IP) { space = new hnswlib::InnerProductSpace(dim); normalize = true; + } else { + KNOWHERE_THROW_MSG("Metric type not supported: " + metric_type); } index_ = std::make_shared>(space, rows, config[IndexParams::M].get(), config[IndexParams::efConstruction].get()); diff --git a/core/src/index/knowhere/knowhere/index/vector_index/IndexRHNSW.cpp b/core/src/index/knowhere/knowhere/index/vector_index/IndexRHNSW.cpp index 6508a342..1da965f8 100644 --- a/core/src/index/knowhere/knowhere/index/vector_index/IndexRHNSW.cpp +++ b/core/src/index/knowhere/knowhere/index/vector_index/IndexRHNSW.cpp @@ -85,9 +85,9 @@ IndexRHNSW::Query(const DatasetPtr& dataset_ptr, const Config& config) { } GET_TENSOR_DATA(dataset_ptr) - size_t k = config[meta::TOPK].get(); - size_t id_size = sizeof(int64_t) * k; - size_t dist_size = sizeof(float) * k; + int64_t k = config[meta::TOPK].get(); + int64_t id_size = sizeof(int64_t) * k; + int64_t dist_size = sizeof(float) * k; auto p_id = (int64_t*)malloc(id_size * rows); auto p_dist = (float*)malloc(dist_size * rows); for (auto i = 0; i < k * rows; ++i) { diff --git a/core/src/index/knowhere/knowhere/index/vector_index/impl/nsg/Distance.h b/core/src/index/knowhere/knowhere/index/vector_index/impl/nsg/Distance.h index 9561d959..ac8d20c5 100644 --- a/core/src/index/knowhere/knowhere/index/vector_index/impl/nsg/Distance.h +++ b/core/src/index/knowhere/knowhere/index/vector_index/impl/nsg/Distance.h @@ -16,6 +16,7 @@ namespace knowhere { namespace impl { struct Distance { + virtual ~Distance() = default; virtual float Compare(const float* a, const float* b, unsigned size) const = 0; }; diff --git a/core/src/index/knowhere/knowhere/index/vector_index/impl/nsg/NSG.cpp b/core/src/index/knowhere/knowhere/index/vector_index/impl/nsg/NSG.cpp index 3e7d52b7..b5be90ae 100644 --- a/core/src/index/knowhere/knowhere/index/vector_index/impl/nsg/NSG.cpp +++ b/core/src/index/knowhere/knowhere/index/vector_index/impl/nsg/NSG.cpp @@ -880,10 +880,10 @@ NsgIndex::GetSize() { ret += ntotal * dimension * sizeof(float); ret += ntotal * sizeof(int64_t); ret += sizeof(*distance_); - for (auto i = 0; i < nsg.size(); ++i) { + for (size_t i = 0; i < nsg.size(); ++i) { ret += nsg[i].size() * sizeof(node_t); } - for (auto i = 0; i < knng.size(); ++i) { + for (size_t i = 0; i < knng.size(); ++i) { ret += knng[i].size() * sizeof(node_t); } return ret; diff --git a/core/src/index/thirdparty/faiss/utils/instruction_set.h b/core/src/index/thirdparty/faiss/utils/instruction_set.h index 90ccc2c3..c2fe84ca 100644 --- a/core/src/index/thirdparty/faiss/utils/instruction_set.h +++ b/core/src/index/thirdparty/faiss/utils/instruction_set.h @@ -84,13 +84,13 @@ class InstructionSet { } // load bitset with flags for function 0x80000001 - if (nExIds_ >= 0x80000001) { + if (nExIds_ >= (int)0x80000001) { f_81_ECX_ = extdata_[1][2]; f_81_EDX_ = extdata_[1][3]; } // Interpret CPU brand string if reported - if (nExIds_ >= 0x80000004) { + if (nExIds_ >= (int)0x80000004) { memcpy(brand, extdata_[2].data(), sizeof(cpui)); memcpy(brand + 16, extdata_[3].data(), sizeof(cpui)); memcpy(brand + 32, extdata_[4].data(), sizeof(cpui)); diff --git a/core/src/index/thirdparty/hnswlib/hnswalg.h b/core/src/index/thirdparty/hnswlib/hnswalg.h index f0805d64..61158a61 100644 --- a/core/src/index/thirdparty/hnswlib/hnswalg.h +++ b/core/src/index/thirdparty/hnswlib/hnswalg.h @@ -1140,7 +1140,7 @@ class HierarchicalNSW : public AlgorithmInterface { ret += element_levels_.size() * sizeof(int); ret += max_elements_ * size_data_per_element_; ret += max_elements_ * sizeof(void*); - for (auto i = 0; i < max_elements_; ++ i) { + for (size_t i = 0; i < max_elements_; ++ i) { ret += linkLists_[i] ? size_links_per_element_ * element_levels_[i] : 0; } return ret; diff --git a/core/src/index/unittest/test_structured_index_sort.cpp b/core/src/index/unittest/test_structured_index_sort.cpp index 13bbdd11..896ab888 100644 --- a/core/src/index/unittest/test_structured_index_sort.cpp +++ b/core/src/index/unittest/test_structured_index_sort.cpp @@ -116,7 +116,7 @@ TEST(STRUCTUREDINDEXSORT_TEST, test_in) { gen_rand_data(range, n, p); milvus::knowhere::StructuredIndexSort structuredIndexSort((size_t)n, p); // Build default - size_t test_times = 10; + int test_times = 10; std::vector test_vals, test_off; test_vals.reserve(test_times); test_off.reserve(test_times); @@ -141,25 +141,25 @@ TEST(STRUCTUREDINDEXSORT_TEST, test_not_in) { gen_rand_data(range, n, p); milvus::knowhere::StructuredIndexSort structuredIndexSort((size_t)n, p); // Build default - size_t test_times = 10; + int test_times = 10; std::vector test_vals, test_off; test_vals.reserve(test_times); test_off.reserve(test_times); - // std::cout << "STRUCTUREDINDEXSORT_TEST test_notin" << std::endl; + // std::cout << "STRUCTUREDINDEXSORT_TEST test_notin" << std::endl; for (auto i = 0; i < test_times; ++i) { auto off = random() % n; test_vals.emplace_back(*(p + off)); test_off.emplace_back(off); - // std::cout << off << " "; + // std::cout << off << " "; } - // std::cout << std::endl; + // std::cout << std::endl; auto res = structuredIndexSort.NotIn(test_times, test_vals.data()); - // std::cout << "assert values: " << std::endl; + // std::cout << "assert values: " << std::endl; for (auto i = 0; i < test_times; ++i) { - // std::cout << test_off[i] << " "; + // std::cout << test_off[i] << " "; ASSERT_EQ(false, res->test(test_off[i])); } - // std::cout << std::endl; + // std::cout << std::endl; free(p); } diff --git a/core/thirdparty/dablooms/dablooms.cpp b/core/thirdparty/dablooms/dablooms.cpp index 3479be20..5dd9d1d3 100644 --- a/core/thirdparty/dablooms/dablooms.cpp +++ b/core/thirdparty/dablooms/dablooms.cpp @@ -1,6 +1,5 @@ /* Copyright @2012 by Justin Hines at Bitly under a very liberal license. See LICENSE in the source distribution. */ -#define _GNU_SOURCE #include #include #include -- GitLab