diff --git a/keyvi/.clang-tidy b/keyvi/.clang-tidy index 994895618..68688bdf2 100644 --- a/keyvi/.clang-tidy +++ b/keyvi/.clang-tidy @@ -16,3 +16,11 @@ Checks: "*, " HeaderFilterRegex: '' FormatStyle: none +CheckOptions: + - key: misc-include-cleaner.IgnoreHeaders + # The following files are ignored when clang-tidy analyzes header + # inclusions: + # - boost detail + value: ' + boost/.*/detail/.*\.hpp; + ' diff --git a/keyvi/include/keyvi/dictionary/fsa/automata.h b/keyvi/include/keyvi/dictionary/fsa/automata.h index 12feca7cd..e85a7df76 100644 --- a/keyvi/include/keyvi/dictionary/fsa/automata.h +++ b/keyvi/include/keyvi/dictionary/fsa/automata.h @@ -91,21 +91,23 @@ class Automata final { explicit Automata(const dictionary_properties_t& dictionary_properties, loading_strategy_types loading_strategy, const bool load_value_store) : dictionary_properties_(dictionary_properties) { - file_mapping_ = boost::interprocess::file_mapping(dictionary_properties_->GetFileName().c_str(), - boost::interprocess::read_only); + boost::interprocess::file_mapping file_mapping = boost::interprocess::file_mapping( + dictionary_properties_->GetFileName().c_str(), boost::interprocess::read_only); const boost::interprocess::map_options_t map_options = internal::MemoryMapFlags::FSAGetMemoryMapOptions(loading_strategy); TRACE("labels start offset: %d", dictionary_properties_->GetPersistenceOffset()); - labels_region_ = boost::interprocess::mapped_region(file_mapping_, boost::interprocess::read_only, - dictionary_properties_->GetPersistenceOffset(), - dictionary_properties_->GetSparseArraySize(), 0, map_options); + labels_region_ = boost::interprocess::mapped_region( + file_mapping, boost::interprocess::read_only, + static_cast(dictionary_properties_->GetPersistenceOffset()), + dictionary_properties_->GetSparseArraySize(), nullptr, map_options); TRACE("transitions start offset: %d", dictionary_properties_->GetTransitionsOffset()); transitions_region_ = boost::interprocess::mapped_region( - file_mapping_, boost::interprocess::read_only, dictionary_properties_->GetTransitionsOffset(), - dictionary_properties_->GetTransitionsSize(), 0, map_options); + file_mapping, boost::interprocess::read_only, + static_cast(dictionary_properties_->GetTransitionsOffset()), + dictionary_properties_->GetTransitionsSize(), nullptr, map_options); const auto advise = internal::MemoryMapFlags::FSAGetMemoryMapAdvices(loading_strategy); @@ -117,7 +119,7 @@ class Automata final { if (load_value_store) { value_store_reader_.reset( - internal::ValueStoreFactory::MakeReader(dictionary_properties_->GetValueStoreType(), &file_mapping_, + internal::ValueStoreFactory::MakeReader(dictionary_properties_->GetValueStoreType(), &file_mapping, dictionary_properties_->GetValueStoreProperties(), loading_strategy)); } } @@ -415,7 +417,6 @@ class Automata final { private: dictionary_properties_t dictionary_properties_; std::unique_ptr value_store_reader_; - boost::interprocess::file_mapping file_mapping_; boost::interprocess::mapped_region labels_region_; boost::interprocess::mapped_region transitions_region_; unsigned char* labels_; diff --git a/keyvi/include/keyvi/dictionary/fsa/internal/memory_map_manager.h b/keyvi/include/keyvi/dictionary/fsa/internal/memory_map_manager.h index 4cff025e9..8e915608d 100644 --- a/keyvi/include/keyvi/dictionary/fsa/internal/memory_map_manager.h +++ b/keyvi/include/keyvi/dictionary/fsa/internal/memory_map_manager.h @@ -26,8 +26,10 @@ #define KEYVI_DICTIONARY_FSA_INTERNAL_MEMORY_MAP_MANAGER_H_ #include +#include #include #include +#include #include #include @@ -59,13 +61,7 @@ class MemoryMapManager final { MemoryMapManager(const size_t chunk_size, const boost::filesystem::path directory, const boost::filesystem::path filename_pattern) : chunk_size_(chunk_size), directory_(directory), filename_pattern_(filename_pattern) {} - - ~MemoryMapManager() { - for (auto& m : mappings_) { - delete m.mapping_; - delete m.region_; - } - } + ~MemoryMapManager() = default; /* Using GetAdress to read multiple bytes is unsafe as it might be a buffer overflow * @@ -82,7 +78,8 @@ class MemoryMapManager final { void* chunk_address = GetChunk(chunk_number); - return (reinterpret_cast(chunk_address) + chunk_offset); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + return static_cast(chunk_address) + chunk_offset; } /* Get a buffer as copy. @@ -97,12 +94,15 @@ class MemoryMapManager final { size_t second_chunk_size = buffer_length - first_chunk_size; void* chunk_address = GetChunk(chunk_number); - std::memcpy(buffer, reinterpret_cast(chunk_address) + chunk_offset, first_chunk_size); + + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + std::memcpy(buffer, static_cast(chunk_address) + chunk_offset, first_chunk_size); if (second_chunk_size > 0) { void* chunk_address_part2 = GetChunk(chunk_number + 1); - std::memcpy(reinterpret_cast(buffer) + first_chunk_size, - reinterpret_cast(chunk_address_part2), second_chunk_size); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + std::memcpy(static_cast(buffer) + first_chunk_size, static_cast(chunk_address_part2), + second_chunk_size); } } @@ -129,8 +129,9 @@ class MemoryMapManager final { size_t copy_size = std::min(remaining, chunk_size_ - chunk_offset); TRACE("copy size: %ld", copy_size); - std::memcpy(reinterpret_cast(chunk_address) + chunk_offset, - reinterpret_cast(buffer) + buffer_offset, copy_size); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + std::memcpy(static_cast(chunk_address) + chunk_offset, static_cast(buffer) + buffer_offset, + copy_size); remaining -= copy_size; tail_ += copy_size; @@ -154,7 +155,8 @@ class MemoryMapManager final { void* chunk_address = GetChunk(chunk_number); size_t first_chunk_size = std::min(buffer_length, chunk_size_ - chunk_offset); - if (std::memcmp(reinterpret_cast(chunk_address) + chunk_offset, buffer, first_chunk_size) != 0) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + if (std::memcmp(static_cast(chunk_address) + chunk_offset, buffer, first_chunk_size) != 0) { return false; } @@ -165,9 +167,10 @@ class MemoryMapManager final { // handle overflow void* chunk_address_part2 = GetChunk(chunk_number + 1); - return (std::memcmp(reinterpret_cast(chunk_address_part2), - reinterpret_cast(buffer) + first_chunk_size, - buffer_length - first_chunk_size) == 0); + + return (std::memcmp(static_cast(chunk_address_part2), + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + static_cast(buffer) + first_chunk_size, buffer_length - first_chunk_size) == 0); } void Write(std::ostream& stream, const size_t end) const { @@ -195,7 +198,7 @@ class MemoryMapManager final { size_t bytes_in_chunk = std::min(chunk_size_, remaining); TRACE("write chunk %d, with size: %ld, remaining: %ld", chunk, bytes_in_chunk, remaining); - const char* ptr = reinterpret_cast(mappings_[chunk].region_->get_address()); + const char* ptr = static_cast(mappings_[chunk].get_address()); stream.write(ptr, bytes_in_chunk); remaining -= bytes_in_chunk; @@ -212,9 +215,7 @@ class MemoryMapManager final { void Persist() { persisted_ = true; for (auto& m : mappings_) { - m.region_->flush(); - delete m.region_; - delete m.mapping_; + m.flush(); } // truncate last file according to the written buffers @@ -236,13 +237,8 @@ class MemoryMapManager final { size_t GetNumberOfChunks() const { return number_of_chunks_; } private: - struct mapping { - boost::interprocess::file_mapping* mapping_; - boost::interprocess::mapped_region* region_; - }; - size_t chunk_size_; - std::vector mappings_; + std::vector mappings_; boost::filesystem::path directory_; boost::filesystem::path filename_pattern_; size_t tail_ = 0; @@ -262,13 +258,11 @@ class MemoryMapManager final { CreateMapping(); } - return mappings_[chunk_number].region_->get_address(); + return mappings_[chunk_number].get_address(); } void CreateMapping() { TRACE("create new mapping %d", number_of_chunks_ + 1); - mapping new_mapping; - boost::filesystem::path filename = GetFilenameForChunk(number_of_chunks_); std::ofstream chunk(filename.string().c_str(), @@ -287,16 +281,15 @@ class MemoryMapManager final { throw memory_map_manager_exception("failed to create chunk (setting size)"); } - new_mapping.mapping_ = - new boost::interprocess::file_mapping(filename.string().c_str(), boost::interprocess::read_write); + boost::interprocess::file_mapping const mapping(filename.string().c_str(), boost::interprocess::read_write); - new_mapping.region_ = - new boost::interprocess::mapped_region(*new_mapping.mapping_, boost::interprocess::read_write); + boost::interprocess::mapped_region mapped_region( + boost::interprocess::mapped_region(mapping, boost::interprocess::read_write)); // prevent pre-fetching pages by the OS which does not make sense as values usually fit into few pages - new_mapping.region_->advise(boost::interprocess::mapped_region::advice_types::advice_random); + mapped_region.advise(boost::interprocess::mapped_region::advice_types::advice_random); - mappings_.push_back(new_mapping); + mappings_.push_back(std::move(mapped_region)); ++number_of_chunks_; } };