PragmaTwice commented on code in PR #1326:
URL: https://github.com/apache/incubator-fury/pull/1326#discussion_r1447349879
##########
src/fury/row/row.cc:
##########
@@ -237,8 +237,8 @@ int *ArrayData::GetDimensions(ArrayData &array, int
num_dims) {
// use deep-first search to search to numDimensions-1 layer to get
dimensions.
int depth = 0;
auto dimensions = new int[num_dims];
- int start_from_lefts[num_dims];
- ArrayData *arrs[num_dims]; // root to current node
+ int *start_from_lefts = new int[num_dims];
+ ArrayData **arrs = new ArrayData *[num_dims]; // root to current node
Review Comment:
Why do you introduce two **memory-leaked allocation** here?
##########
src/fury/thirdparty/MurmurHash3.cc:
##########
@@ -14,14 +14,22 @@
// Microsoft Visual Studio
+inline uint32_t rotl32(uint32_t x, int8_t r) {
+ return (x << r) | (x >> (32 - r));
+}
+
+inline uint64_t rotl64(uint64_t x, int8_t r) {
+ return (x << r) | (x >> (64 - r));
+}
+
#if defined(_MSC_VER)
#define FORCE_INLINE __forceinline
#include <cstdint>
-#define ROTL32(x, y) _rotl(x, y)
-#define ROTL64(x, y) _rotl64(x, y)
+#define ROTL32(x, y) rotl32(x, y)
+#define ROTL64(x, y) rotl64(x, y)
Review Comment:
Could you explain this change? I don't think it need to be changed.
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/rotl-rotl64-rotr-rotr64?view=msvc-170
##########
src/fury/meta/preprocessor.h:
##########
@@ -25,8 +25,15 @@
// passed to the macros are fully expanded before they are concatenated.
#define FURY_PP_CONCAT(a, b) FURY_PP_CONCAT_IMPL(a, b)
+#ifdef _WIN32
+#define EXTEND(...) __VA_ARGS__
+#define FURY_PP_NARG_IMPL(...) EXTEND(FURY_PP_NARG_CALC(__VA_ARGS__))
+#define FURY_PP_NARG(...)
\
+ EXTEND(FURY_PP_NARG_IMPL(__VA_ARGS__, FURY_PP_NARG_REV()))
+#else
Review Comment:
Could you explain these changes?
##########
src/fury/meta/field_info.h:
##########
@@ -85,3 +86,22 @@ template <typename FieldInfo> constexpr bool
IsValidFieldInfo() {
inline constexpr auto FuryFieldInfo(const type &) noexcept {
\
return FuryFieldInfoImpl<type>{};
\
};
+#else
+#define FURY_FIELD_INFO(type, ...)
\
+ static_assert(std::is_class_v<type>, "it must be a class type");
\
+ template <typename> struct FuryFieldInfoImpl;
\
+ template <> struct FuryFieldInfoImpl<type> {
\
+ static inline constexpr size_t Size = FURY_PP_NARG(__VA_ARGS__);
\
+ static inline constexpr std::string_view Name = #type;
\
+ static inline constexpr std::array<std::string_view, Size> Names = {
\
+ EXTEND(FURY_PP_FOREACH(FURY_FIELD_INFO_NAMES_FUNC, __VA_ARGS__))};
\
+ static inline constexpr auto Ptrs = std::tuple{EXTEND(
\
+ FURY_PP_FOREACH_1(FURY_FIELD_INFO_PTRS_FUNC, type, __VA_ARGS__))};
\
+ };
\
+ static_assert(
\
+ fury::meta::IsValidFieldInfo<FuryFieldInfoImpl<type>>(),
\
+ "duplicated fields in FURY_FIELD_INFO arguments are detected");
\
+ inline constexpr auto FuryFieldInfo(const type &) noexcept {
\
+ return FuryFieldInfoImpl<type>{};
\
+ };
+#endif
Review Comment:
ditto
--
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]
