cmake: enable Wall

As pointed out in #2536, -Wall is not enabled. -Wextra will follow in a separate PR. The main changes needed are: - Consistent use of either struct/class for a symbol, not a mix. - Don't call `std::move()` on temporaries, the compiler will elide the copy anyway. - Constructor initialization order should be the same as the member order. It turns out that C++ will always initialize the fields in the member order, regardless of the initialization list ordering. - Mark functions in header files as inline static, or just inline.
kuzudb · Dec 4, 2023 · c01e1b9 · c01e1b9
1 parent 97b78f8
commit c01e1b9
Show file tree

Hide file tree

Showing 47 changed files with 113 additions and 121 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -108,6 +108,9 @@ if(CMAKE_BUILD_TYPE MATCHES Release)
         set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3")
     endif()
 endif()
+if(NOT MSVC)
+    add_compile_options(-Wall)
+endif()
 
 if(${ENABLE_THREAD_SANITIZER})
     if(MSVC)

diff --git a/src/binder/bind/bind_copy.cpp b/src/binder/bind/bind_copy.cpp
@@ -40,7 +40,7 @@ std::unique_ptr<BoundStatement> Binder::bindCopyToClause(const Statement& statem
     }
     auto csvConfig = bindParsingOptions(copyToStatement.getParsingOptionsRef());
     return std::make_unique<BoundCopyTo>(boundFilePath, fileType, std::move(columnNames),
-        std::move(columnTypes), std::move(query), std::move(csvConfig->option.copy()));
+        std::move(columnTypes), std::move(query), csvConfig->option.copy());
 }
 
 // As a temporary constraint, we require npy files loaded with COPY FROM BY COLUMN keyword.

diff --git a/src/common/arrow/arrow_converter.cpp b/src/common/arrow/arrow_converter.cpp
@@ -191,7 +191,7 @@ std::unique_ptr<ArrowSchema> ArrowConverter::toArrowSchema(
 
     outSchema->private_data = rootHolder.release();
     outSchema->release = releaseArrowSchema;
-    return std::move(outSchema);
+    return outSchema;
 }
 
 void ArrowConverter::toArrowArray(

diff --git a/src/common/arrow/arrow_row_batch.cpp b/src/common/arrow/arrow_row_batch.cpp
@@ -187,7 +187,7 @@ std::unique_ptr<ArrowVector> ArrowRowBatch::createVector(
         KU_UNREACHABLE;
     }
     }
-    return std::move(result);
+    return result;
 }
 
 static void getBitPosition(std::int64_t pos, std::int64_t& bytePos, std::int64_t& bitOffset) {
@@ -580,7 +580,7 @@ static std::unique_ptr<ArrowArray> createArrayFromVector(ArrowVector& vector) {
         result->n_buffers++;
         result->buffers[1] = vector.data.data();
     }
-    return std::move(result);
+    return result;
 }
 
 template<LogicalTypeID DT>

diff --git a/src/common/types/blob.cpp b/src/common/types/blob.cpp
@@ -48,10 +48,10 @@ uint64_t Blob::fromString(const char* str, uint64_t length, uint8_t* resultBuffe
     for (auto i = 0u; i < length; i++) {
         if (str[i] == '\\') {
             validateHexCode(reinterpret_cast<const uint8_t*>(str), length, i);
-            auto firstByte =
-                HexFormatConstants::HEX_MAP[str[i + HexFormatConstants::FIRST_BYTE_POS]];
-            auto secondByte =
-                HexFormatConstants::HEX_MAP[str[i + HexFormatConstants::SECOND_BYTES_POS]];
+            auto firstByte = HexFormatConstants::HEX_MAP[(
+                unsigned char)str[i + HexFormatConstants::FIRST_BYTE_POS]];
+            auto secondByte = HexFormatConstants::HEX_MAP[(
+                unsigned char)str[i + HexFormatConstants::SECOND_BYTES_POS]];
             resultBuffer[resultPos++] =
                 (firstByte << HexFormatConstants::NUM_BYTES_TO_SHIFT_FOR_FIRST_BYTE) + secondByte;
             i += HexFormatConstants::LENGTH - 1;

diff --git a/src/expression_evaluator/case_evaluator.cpp b/src/expression_evaluator/case_evaluator.cpp
@@ -73,7 +73,7 @@ std::unique_ptr<ExpressionEvaluator> CaseExpressionEvaluator::clone() {
         clonedAlternativeEvaluators.push_back(alternative->clone());
     }
     return make_unique<CaseExpressionEvaluator>(
-        expression, std::move(clonedAlternativeEvaluators), std::move(elseEvaluator->clone()));
+        expression, std::move(clonedAlternativeEvaluators), elseEvaluator->clone());
 }
 
 void CaseExpressionEvaluator::resolveResultVector(
@@ -101,7 +101,7 @@ void CaseExpressionEvaluator::fillEntry(sel_t resultPos, const ValueVector& then
     } else {
         if (thenVector.dataType.getLogicalTypeID() == LogicalTypeID::VAR_LIST) {
             auto srcListEntry = thenVector.getValue<list_entry_t>(thenPos);
-            list_entry_t resultEntry = ListVector::addList(resultVector.get(), srcListEntry.size);
+            ListVector::addList(resultVector.get(), srcListEntry.size);
             resultVector->copyFromVectorData(resultPos, &thenVector, thenPos);
         } else {
             auto val = thenVector.getValue<T>(thenPos);

diff --git a/src/function/cast/cast_fixed_list.cpp b/src/function/cast/cast_fixed_list.cpp
@@ -30,7 +30,6 @@ bool CastFixedListHelper::containsListToFixedList(
                         dstType->toString())};
             }
 
-            auto result = false;
             std::vector<struct_field_idx_t> fields;
             for (auto i = 0u; i < srcFieldTypes.size(); i++) {
                 if (containsListToFixedList(srcFieldTypes[i], dstFieldTypes[i])) {

diff --git a/src/include/binder/binder.h b/src/include/binder/binder.h
@@ -19,17 +19,17 @@ class ClientContext;
 }
 
 namespace function {
-class TableFunction;
+struct TableFunction;
 }
 
 namespace binder {
 
-class BoundInsertInfo;
-class BoundSetPropertyInfo;
-class BoundDeleteInfo;
+struct BoundInsertInfo;
+struct BoundSetPropertyInfo;
+struct BoundDeleteInfo;
 class BoundWithClause;
 class BoundReturnClause;
-class BoundFileScanInfo;
+struct BoundFileScanInfo;
 
 // BinderScope keeps track of expressions in scope and their aliases. We maintain the order of
 // expressions in

diff --git a/src/include/binder/expression_binder.h b/src/include/binder/expression_binder.h
@@ -11,7 +11,7 @@ class Value;
 namespace binder {
 
 class Binder;
-class CaseAlternative;
+struct CaseAlternative;
 
 class ExpressionBinder {
     friend class Binder;

diff --git a/src/include/binder/query/updating_clause/bound_delete_info.h b/src/include/binder/query/updating_clause/bound_delete_info.h
@@ -14,11 +14,11 @@ struct BoundDeleteInfo {
 
     BoundDeleteInfo(UpdateTableType updateTableType, std::shared_ptr<Expression> nodeOrRel,
         common::DeleteClauseType deleteClauseType)
-        : updateTableType{updateTableType}, nodeOrRel{std::move(nodeOrRel)},
-          deleteClauseType{deleteClauseType} {}
+        : deleteClauseType{deleteClauseType}, updateTableType{updateTableType}, nodeOrRel{std::move(
+                                                                                    nodeOrRel)} {}
     BoundDeleteInfo(const BoundDeleteInfo& other)
-        : updateTableType{other.updateTableType}, nodeOrRel{other.nodeOrRel},
-          deleteClauseType{other.deleteClauseType} {}
+        : deleteClauseType{other.deleteClauseType},
+          updateTableType{other.updateTableType}, nodeOrRel{other.nodeOrRel} {}
 
     inline std::unique_ptr<BoundDeleteInfo> copy() {
         return std::make_unique<BoundDeleteInfo>(*this);

diff --git a/src/include/common/utils.h b/src/include/common/utils.h
@@ -33,7 +33,7 @@ class BitmaskUtils {
     }
 };
 
-static uint64_t nextPowerOfTwo(uint64_t v) {
+inline static uint64_t nextPowerOfTwo(uint64_t v) {
     v--;
     v |= v >> 1;
     v |= v >> 2;
@@ -45,7 +45,7 @@ static uint64_t nextPowerOfTwo(uint64_t v) {
     return v;
 }
 
-static bool isLittleEndian() {
+inline static bool isLittleEndian() {
     // Little endian arch stores the least significant value in the lower bytes.
     int testNumber = 1;
     return *(uint8_t*)&testNumber == 1;

diff --git a/src/include/function/aggregate_function.h b/src/include/function/aggregate_function.h
@@ -9,8 +9,6 @@
 namespace kuzu {
 namespace function {
 
-class AggregateFunction;
-
 struct AggregateState {
     virtual inline uint32_t getStateSize() const = 0;
     virtual void moveResultToVector(common::ValueVector* outputVector, uint64_t pos) = 0;
@@ -37,10 +35,10 @@ struct AggregateFunction final : public BaseScalarFunction {
         scalar_bind_func bindFunc = nullptr, param_rewrite_function_t paramRewriteFunc = nullptr)
         : BaseScalarFunction{FunctionType::AGGREGATE, std::move(name), std::move(parameterTypeIDs),
               returnTypeID, std::move(bindFunc)},
-          initializeFunc{std::move(initializeFunc)}, updateAllFunc{std::move(updateAllFunc)},
-          updatePosFunc{std::move(updatePosFunc)}, combineFunc{std::move(combineFunc)},
-          finalizeFunc{std::move(finalizeFunc)}, isDistinct{isDistinct}, paramRewriteFunc{std::move(
-                                                                             paramRewriteFunc)} {
+          isDistinct{isDistinct}, initializeFunc{std::move(initializeFunc)},
+          updateAllFunc{std::move(updateAllFunc)}, updatePosFunc{std::move(updatePosFunc)},
+          combineFunc{std::move(combineFunc)}, finalizeFunc{std::move(finalizeFunc)},
+          paramRewriteFunc{std::move(paramRewriteFunc)} {
         initialNullAggregateState = createInitialNullAggregateState();
     }
 

diff --git a/src/include/function/cast/functions/cast_string_non_nested_functions.h b/src/include/function/cast/functions/cast_string_non_nested_functions.h
@@ -169,7 +169,7 @@ static bool trySimpleInt128Cast(const char* input, uint64_t len, int128_t& resul
     return false;
 }
 
-static void simpleInt128Cast(const char* input, uint64_t len, int128_t& result) {
+static inline void simpleInt128Cast(const char* input, uint64_t len, int128_t& result) {
     if (!trySimpleInt128Cast(input, len, result)) {
         throw ConversionException(
             stringFormat("Cast failed. {} is not within INT128 range.", std::string{input, len}));

diff --git a/src/include/function/cast/functions/numeric_cast.h b/src/include/function/cast/functions/numeric_cast.h
@@ -9,7 +9,7 @@ namespace kuzu {
 namespace function {
 
 template<class SRC, class DST>
-static bool tryCastWithOverflowCheck(SRC value, DST& result) {
+inline bool tryCastWithOverflowCheck(SRC value, DST& result) {
     if (NumericLimits<SRC>::isSigned() != NumericLimits<DST>::isSigned()) {
         if (NumericLimits<SRC>::isSigned()) {
             if (NumericLimits<SRC>::digits() > NumericLimits<DST>::digits()) {
@@ -53,7 +53,7 @@ static bool tryCastWithOverflowCheck(SRC value, DST& result) {
 }
 
 template<class SRC, class T>
-bool tryCastWithOverflowCheckFloat(SRC value, T& result, SRC min, SRC max) {
+inline bool tryCastWithOverflowCheckFloat(SRC value, T& result, SRC min, SRC max) {
     if (!(value >= min && value < max)) {
         return false;
     }
@@ -63,99 +63,99 @@ bool tryCastWithOverflowCheckFloat(SRC value, T& result, SRC min, SRC max) {
 }
 
 template<>
-bool tryCastWithOverflowCheck(float value, int8_t& result) {
+inline bool tryCastWithOverflowCheck(float value, int8_t& result) {
     return tryCastWithOverflowCheckFloat<float, int8_t>(value, result, -128.0f, 128.0f);
 }
 
 template<>
-bool tryCastWithOverflowCheck(float value, int16_t& result) {
+inline bool tryCastWithOverflowCheck(float value, int16_t& result) {
     return tryCastWithOverflowCheckFloat<float, int16_t>(value, result, -32768.0f, 32768.0f);
 }
 
 template<>
-bool tryCastWithOverflowCheck(float value, int32_t& result) {
+inline bool tryCastWithOverflowCheck(float value, int32_t& result) {
     return tryCastWithOverflowCheckFloat<float, int32_t>(
         value, result, -2147483648.0f, 2147483648.0f);
 }
 
 template<>
-bool tryCastWithOverflowCheck(float value, int64_t& result) {
+inline bool tryCastWithOverflowCheck(float value, int64_t& result) {
     return tryCastWithOverflowCheckFloat<float, int64_t>(
         value, result, -9223372036854775808.0f, 9223372036854775808.0f);
 }
 
 template<>
-bool tryCastWithOverflowCheck(float value, uint8_t& result) {
+inline bool tryCastWithOverflowCheck(float value, uint8_t& result) {
     return tryCastWithOverflowCheckFloat<float, uint8_t>(value, result, 0.0f, 256.0f);
 }
 
 template<>
-bool tryCastWithOverflowCheck(float value, uint16_t& result) {
+inline bool tryCastWithOverflowCheck(float value, uint16_t& result) {
     return tryCastWithOverflowCheckFloat<float, uint16_t>(value, result, 0.0f, 65536.0f);
 }
 
 template<>
-bool tryCastWithOverflowCheck(float value, uint32_t& result) {
+inline bool tryCastWithOverflowCheck(float value, uint32_t& result) {
     return tryCastWithOverflowCheckFloat<float, uint32_t>(value, result, 0.0f, 4294967296.0f);
 }
 
 template<>
-bool tryCastWithOverflowCheck(float value, uint64_t& result) {
+inline bool tryCastWithOverflowCheck(float value, uint64_t& result) {
     return tryCastWithOverflowCheckFloat<float, uint64_t>(
         value, result, 0.0f, 18446744073709551616.0f);
 }
 
 template<>
-bool tryCastWithOverflowCheck(double value, int8_t& result) {
+inline bool tryCastWithOverflowCheck(double value, int8_t& result) {
     return tryCastWithOverflowCheckFloat<double, int8_t>(value, result, -128.0, 128.0);
 }
 
 template<>
-bool tryCastWithOverflowCheck(double value, int16_t& result) {
+inline bool tryCastWithOverflowCheck(double value, int16_t& result) {
     return tryCastWithOverflowCheckFloat<double, int16_t>(value, result, -32768.0, 32768.0);
 }
 
 template<>
-bool tryCastWithOverflowCheck(double value, int32_t& result) {
+inline bool tryCastWithOverflowCheck(double value, int32_t& result) {
     return tryCastWithOverflowCheckFloat<double, int32_t>(
         value, result, -2147483648.0, 2147483648.0);
 }
 
 template<>
-bool tryCastWithOverflowCheck(double value, int64_t& result) {
+inline bool tryCastWithOverflowCheck(double value, int64_t& result) {
     return tryCastWithOverflowCheckFloat<double, int64_t>(
         value, result, -9223372036854775808.0, 9223372036854775808.0);
 }
 
 template<>
-bool tryCastWithOverflowCheck(double value, uint8_t& result) {
+inline bool tryCastWithOverflowCheck(double value, uint8_t& result) {
     return tryCastWithOverflowCheckFloat<double, uint8_t>(value, result, 0.0, 256.0);
 }
 
 template<>
-bool tryCastWithOverflowCheck(double value, uint16_t& result) {
+inline bool tryCastWithOverflowCheck(double value, uint16_t& result) {
     return tryCastWithOverflowCheckFloat<double, uint16_t>(value, result, 0.0, 65536.0);
 }
 
 template<>
-bool tryCastWithOverflowCheck(double value, uint32_t& result) {
+inline bool tryCastWithOverflowCheck(double value, uint32_t& result) {
     return tryCastWithOverflowCheckFloat<double, uint32_t>(value, result, 0.0, 4294967296.0);
 }
 
 template<>
-bool tryCastWithOverflowCheck(double value, uint64_t& result) {
+inline bool tryCastWithOverflowCheck(double value, uint64_t& result) {
     return tryCastWithOverflowCheckFloat<double, uint64_t>(
         value, result, 0.0, 18446744073709551615.0);
 }
 
 template<>
-bool tryCastWithOverflowCheck(float input, double& result) {
+inline bool tryCastWithOverflowCheck(float input, double& result) {
     result = double(input);
     return true;
 }
 
 template<>
-bool tryCastWithOverflowCheck(double input, float& result) {
+inline bool tryCastWithOverflowCheck(double input, float& result) {
     result = float(input);
     return true;
 }

diff --git a/src/include/function/table_functions/call_functions.h b/src/include/function/table_functions/call_functions.h
@@ -58,8 +58,8 @@ struct CurrentSettingBindData : public CallTableFuncBindData {
     CurrentSettingBindData(std::string result,
         std::vector<std::unique_ptr<common::LogicalType>> returnTypes,
         std::vector<std::string> returnColumnNames, common::offset_t maxOffset)
-        : result{std::move(result)}, CallTableFuncBindData{std::move(returnTypes),
-                                         std::move(returnColumnNames), maxOffset} {}
+        : CallTableFuncBindData{std::move(returnTypes), std::move(returnColumnNames), maxOffset},
+          result{std::move(result)} {}
 
     inline std::unique_ptr<TableFuncBindData> copy() override {
         return std::make_unique<CurrentSettingBindData>(
@@ -91,8 +91,8 @@ struct ShowTablesBindData : public CallTableFuncBindData {
     ShowTablesBindData(std::vector<catalog::TableSchema*> tables,
         std::vector<std::unique_ptr<common::LogicalType>> returnTypes,
         std::vector<std::string> returnColumnNames, common::offset_t maxOffset)
-        : tables{std::move(tables)}, CallTableFuncBindData{std::move(returnTypes),
-                                         std::move(returnColumnNames), maxOffset} {}
+        : CallTableFuncBindData{std::move(returnTypes), std::move(returnColumnNames), maxOffset},
+          tables{std::move(tables)} {}
 
     inline std::unique_ptr<TableFuncBindData> copy() override {
         return std::make_unique<ShowTablesBindData>(
@@ -115,8 +115,8 @@ struct TableInfoBindData : public CallTableFuncBindData {
     TableInfoBindData(catalog::TableSchema* tableSchema,
         std::vector<std::unique_ptr<common::LogicalType>> returnTypes,
         std::vector<std::string> returnColumnNames, common::offset_t maxOffset)
-        : tableSchema{tableSchema}, CallTableFuncBindData{std::move(returnTypes),
-                                        std::move(returnColumnNames), maxOffset} {}
+        : CallTableFuncBindData{std::move(returnTypes), std::move(returnColumnNames), maxOffset},
+          tableSchema{tableSchema} {}
 
     inline std::unique_ptr<TableFuncBindData> copy() override {
         return std::make_unique<TableInfoBindData>(
@@ -139,8 +139,9 @@ struct ShowConnectionBindData : public TableInfoBindData {
     ShowConnectionBindData(catalog::Catalog* catalog, catalog::TableSchema* tableSchema,
         std::vector<std::unique_ptr<common::LogicalType>> returnTypes,
         std::vector<std::string> returnColumnNames, common::offset_t maxOffset)
-        : catalog{catalog}, TableInfoBindData{tableSchema, std::move(returnTypes),
-                                std::move(returnColumnNames), maxOffset} {}
+        : TableInfoBindData{tableSchema, std::move(returnTypes), std::move(returnColumnNames),
+              maxOffset},
+          catalog{catalog} {}
 
     inline std::unique_ptr<TableFuncBindData> copy() override {
         return std::make_unique<ShowConnectionBindData>(

diff --git a/src/include/main/client_context.h b/src/include/main/client_context.h
@@ -37,10 +37,10 @@ class ClientContext {
     friend class binder::Binder;
     friend class testing::TinySnbDDLTest;
     friend class testing::TinySnbCopyCSVTransactionTest;
-    friend class ThreadsSetting;
-    friend class TimeoutSetting;
-    friend class VarLengthExtendMaxDepthSetting;
-    friend class EnableSemiMaskSetting;
+    friend struct ThreadsSetting;
+    friend struct TimeoutSetting;
+    friend struct VarLengthExtendMaxDepthSetting;
+    friend struct EnableSemiMaskSetting;
 
 public:
     explicit ClientContext(Database* database);

diff --git a/src/include/parser/copy.h b/src/include/parser/copy.h
@@ -45,8 +45,8 @@ class CopyFrom : public Copy {
 class CopyTo : public Copy {
 public:
     CopyTo(std::string filePath, std::unique_ptr<RegularQuery> regularQuery)
-        : Copy{common::StatementType::COPY_TO},
-          regularQuery{std::move(regularQuery)}, filePath{std::move(filePath)} {}
+        : Copy{common::StatementType::COPY_TO}, filePath{std::move(filePath)},
+          regularQuery{std::move(regularQuery)} {}
 
     inline std::string getFilePath() const { return filePath; }
     inline RegularQuery* getRegularQuery() const { return regularQuery.get(); }