From 170fdf2a71d6b70d0f67ddebc3158c9cd09b28af Mon Sep 17 00:00:00 2001 From: Keenan Gugeler Date: Fri, 24 Nov 2023 15:25:58 -0500 Subject: [PATCH] tidy: check for non-virtual destructors The check implements [this guideline][1]. A base class' destructor should either be public and virtual, meaning that the object can be destroyed through a pointer to the base class (e.g. a unique_ptr), or protected and non-virtual, meaning that the destructor cannot be invoked on a pointer to the base class. Private destructors would mean that children can't invoke the parent destructor, which is undesirable. [1]: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-virtual --- .clang-tidy | 2 ++ src/include/processor/operator/aggregate/base_aggregate.h | 2 +- src/include/processor/operator/aggregate/hash_aggregate.h | 2 +- src/include/processor/operator/aggregate/simple_aggregate.h | 2 +- src/include/processor/operator/filtering_operator.h | 1 + src/include/processor/operator/mask.h | 1 + src/include/processor/operator/persistent/reader/csv/driver.h | 1 + src/include/storage/buffer_manager/bm_file_handle.h | 2 +- src/include/storage/file_handle.h | 1 + 9 files changed, 10 insertions(+), 4 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index b04a6cfdc7..aabba2e629 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -12,6 +12,8 @@ Checks: concurrency-*, + cppcoreguidelines-virtual-class-destructor, + misc-unused-parameters, modernize-use-override, diff --git a/src/include/processor/operator/aggregate/base_aggregate.h b/src/include/processor/operator/aggregate/base_aggregate.h index e52136bde3..f592582ca9 100644 --- a/src/include/processor/operator/aggregate/base_aggregate.h +++ b/src/include/processor/operator/aggregate/base_aggregate.h @@ -14,7 +14,7 @@ class BaseAggregateSharedState { virtual std::pair getNextRangeToRead() = 0; - virtual ~BaseAggregateSharedState() {} + ~BaseAggregateSharedState() {} protected: std::mutex mtx; diff --git a/src/include/processor/operator/aggregate/hash_aggregate.h b/src/include/processor/operator/aggregate/hash_aggregate.h index 9fe2e96b75..fd6915ff39 100644 --- a/src/include/processor/operator/aggregate/hash_aggregate.h +++ b/src/include/processor/operator/aggregate/hash_aggregate.h @@ -6,7 +6,7 @@ namespace kuzu { namespace processor { -class HashAggregateSharedState : public BaseAggregateSharedState { +class HashAggregateSharedState final : public BaseAggregateSharedState { public: explicit HashAggregateSharedState( diff --git a/src/include/processor/operator/aggregate/simple_aggregate.h b/src/include/processor/operator/aggregate/simple_aggregate.h index da6f6f4508..97f0a8fb23 100644 --- a/src/include/processor/operator/aggregate/simple_aggregate.h +++ b/src/include/processor/operator/aggregate/simple_aggregate.h @@ -6,7 +6,7 @@ namespace kuzu { namespace processor { -class SimpleAggregateSharedState : public BaseAggregateSharedState { +class SimpleAggregateSharedState final : public BaseAggregateSharedState { public: explicit SimpleAggregateSharedState( const std::vector>& aggregateFunctions); diff --git a/src/include/processor/operator/filtering_operator.h b/src/include/processor/operator/filtering_operator.h index c3bb397564..09a74e2698 100644 --- a/src/include/processor/operator/filtering_operator.h +++ b/src/include/processor/operator/filtering_operator.h @@ -11,6 +11,7 @@ class SelVectorOverWriter { currentSelVector = std::make_shared(common::DEFAULT_VECTOR_CAPACITY); } + virtual ~SelVectorOverWriter() = default; protected: void restoreSelVector(std::shared_ptr& selVector); diff --git a/src/include/processor/operator/mask.h b/src/include/processor/operator/mask.h index 4de2bf83cc..a7871261a8 100644 --- a/src/include/processor/operator/mask.h +++ b/src/include/processor/operator/mask.h @@ -66,6 +66,7 @@ class MaskCollection { class NodeSemiMask { public: explicit NodeSemiMask(storage::NodeTable* nodeTable) : nodeTable{nodeTable} {} + virtual ~NodeSemiMask() = default; virtual void init(transaction::Transaction* trx) = 0; diff --git a/src/include/processor/operator/persistent/reader/csv/driver.h b/src/include/processor/operator/persistent/reader/csv/driver.h index c3f6105aa2..81c40c9339 100644 --- a/src/include/processor/operator/persistent/reader/csv/driver.h +++ b/src/include/processor/operator/persistent/reader/csv/driver.h @@ -15,6 +15,7 @@ class ParsingDriver { public: ParsingDriver(common::DataChunk& chunk); + virtual ~ParsingDriver() = default; bool done(uint64_t rowNum); void addValue(uint64_t rowNum, common::column_id_t columnIdx, std::string_view value); diff --git a/src/include/storage/buffer_manager/bm_file_handle.h b/src/include/storage/buffer_manager/bm_file_handle.h index 924a601aea..28da403278 100644 --- a/src/include/storage/buffer_manager/bm_file_handle.h +++ b/src/include/storage/buffer_manager/bm_file_handle.h @@ -141,7 +141,7 @@ class BMFileHandle : public FileHandle { BMFileHandle(const std::string& path, uint8_t flags, BufferManager* bm, common::PageSizeClass pageSizeClass, FileVersionedType fileVersionedType); - ~BMFileHandle(); + ~BMFileHandle() override; // This function assumes the page is already LOCKED. inline void setLockedPageDirty(common::page_idx_t pageIdx) { diff --git a/src/include/storage/file_handle.h b/src/include/storage/file_handle.h index 8d9bd5b1bc..de481dfc39 100644 --- a/src/include/storage/file_handle.h +++ b/src/include/storage/file_handle.h @@ -28,6 +28,7 @@ class FileHandle { constexpr static uint8_t O_IN_MEM_TEMP_FILE{0b0000'0011}; FileHandle(const std::string& path, uint8_t flags); + virtual ~FileHandle() = default; common::page_idx_t addNewPage(); common::page_idx_t addNewPages(common::page_idx_t numPages);