Copilot commented on code in PR #858:
URL: https://github.com/apache/incubator-graphar/pull/858#discussion_r2844113640
##########
cpp/src/graphar/expression.h:
##########
@@ -307,51 +307,51 @@ template <typename T,
std::is_same_v<T, const char*> ||
std::is_same_v<T, const char* const>,
typename = std::enable_if_t<IsScalar>>
-[[nodiscard]] static inline std::shared_ptr<Expression> _Literal(T value) {
+[[nodiscard]] static std::shared_ptr<Expression> _Literal(T value) {
return std::make_shared<ExpressionLiteral<T>>(value);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _Not(
+[[nodiscard]] static std::shared_ptr<Expression> _Not(
std::shared_ptr<Expression> expr) {
return std::make_shared<ExpressionNot>(expr);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _Equal(
+[[nodiscard]] static std::shared_ptr<Expression> _Equal(
std::shared_ptr<Expression> lhs, std::shared_ptr<Expression> rhs) {
return std::make_shared<ExpressionEqual>(lhs, rhs);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _NotEqual(
+[[nodiscard]] static std::shared_ptr<Expression> _NotEqual(
std::shared_ptr<Expression> lhs, std::shared_ptr<Expression> rhs) {
return std::make_shared<ExpressionNotEqual>(lhs, rhs);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _GreaterThan(
+[[nodiscard]] static std::shared_ptr<Expression> _GreaterThan(
std::shared_ptr<Expression> lhs, std::shared_ptr<Expression> rhs) {
return std::make_shared<ExpressionGreaterThan>(lhs, rhs);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _GreaterEqual(
+[[nodiscard]] static std::shared_ptr<Expression> _GreaterEqual(
std::shared_ptr<Expression> lhs, std::shared_ptr<Expression> rhs) {
return std::make_shared<ExpressionGreaterEqual>(lhs, rhs);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _LessThan(
+[[nodiscard]] static std::shared_ptr<Expression> _LessThan(
std::shared_ptr<Expression> lhs, std::shared_ptr<Expression> rhs) {
return std::make_shared<ExpressionLessThan>(lhs, rhs);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _LessEqual(
+[[nodiscard]] static std::shared_ptr<Expression> _LessEqual(
std::shared_ptr<Expression> lhs, std::shared_ptr<Expression> rhs) {
return std::make_shared<ExpressionLessEqual>(lhs, rhs);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _And(
+[[nodiscard]] static std::shared_ptr<Expression> _And(
std::shared_ptr<Expression> lhs, std::shared_ptr<Expression> rhs) {
return std::make_shared<ExpressionAnd>(lhs, rhs);
}
-[[nodiscard]] static inline std::shared_ptr<Expression> _Or(
+[[nodiscard]] static std::shared_ptr<Expression> _Or(
std::shared_ptr<Expression> lhs, std::shared_ptr<Expression> rhs) {
return std::make_shared<ExpressionOr>(lhs, rhs);
}
Review Comment:
The removal of `inline` from these static namespace-level helper functions
(lines 292-357) doesn't align with the PR description's rationale. The cited
C++ standard section (class.mfct#1) only applies to member functions defined
within class declarations. These are static functions at namespace scope in a
header file, where removing inline causes each translation unit to get its own
copy of the functions. Consider keeping `inline` for these functions.
##########
cpp/CMakeLists.txt:
##########
@@ -614,7 +614,11 @@ if (BUILD_BENCHMARKS)
add_executable(${target} ${add_test_SRCS})
target_compile_features(${target} PRIVATE cxx_std_17)
target_include_directories(${target} PRIVATE
${PROJECT_SOURCE_DIR}/thirdparty)
- target_link_libraries(${target} PRIVATE benchmark::benchmark_main
graphar ${CMAKE_DL_LIBS})
+ if(APPLE)
+ target_link_libraries(${target} PRIVATE benchmark::benchmark_main
graphar Arrow::arrow_shared Parquet::parquet_shared ${CMAKE_DL_LIBS})
+ else()
+ target_link_libraries(${target} PRIVATE benchmark::benchmark_main
graphar parquet arrow ${CMAKE_DL_LIBS})
+ endif()
Review Comment:
The changes to the benchmark linking configuration don't align with the
stated PR purpose of "removing redundant inline keywords and fixing typos."
This appears to be an unrelated build system change that adjusts how Arrow and
Parquet libraries are linked for benchmarks on different platforms (using
namespaced targets on Apple vs. plain library names on other platforms). This
change should either be documented in the PR description or moved to a separate
PR focused on build system improvements.
##########
cpp/src/graphar/high-level/edges_builder.h:
##########
@@ -385,6 +384,9 @@ class EdgesBuilder {
* @return The vertex chunk index of the edge.
*/
IdType getVertexChunkIndex(const Edge& e) {
+ if (vertex_chunk_size_ == 0) {
+ return 0;
+ }
Review Comment:
This division by zero check is a bug fix that doesn't align with the stated
PR purpose of "removing redundant inline keywords and fixing typos." While this
is a good defensive programming practice to prevent potential crashes, it
should be documented in the PR description or moved to a separate bug fix PR.
The fix prevents division by zero when vertex_chunk_size_ is 0, but it's
unclear if returning 0 is the correct behavior in this case or if it should be
an error.
##########
cpp/src/graphar/label.h:
##########
@@ -48,7 +48,7 @@ enum QUERY_TYPE {
};
/// Set bit
-static inline void SetBitmap(uint64_t* bitmap, const int index) {
+static void SetBitmap(uint64_t* bitmap, const int index) {
Review Comment:
The removal of `inline` from this static namespace-level function doesn't
align with the PR description's rationale. The cited C++ standard section
(class.mfct#1) only applies to member functions defined within class
declarations. This is a static function at namespace scope in a header file,
where removing inline causes each translation unit to get its own copy of the
function. Consider keeping `inline` for this function.
```suggestion
static inline void SetBitmap(uint64_t* bitmap, const int index) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
