feat(C++): Enable Windows C++ CI and fix some compilation errors by LiangliangSui · Pull Request #1326 · apache/fory

LiangliangSui · 2024-01-10T12:29:34Z

Use windows-2022 in CI and fix some C++ compilation errors in windows.

LiangliangSui · 2024-01-10T12:37:15Z

Windows C++ CI has passed. PTAL!

PragmaTwice · 2024-01-10T12:53:42Z

src/fury/thirdparty/MurmurHash3.cc

-#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)


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

Sorry, a missing header file caused a compilation error. I will modify it back.

PragmaTwice · 2024-01-10T12:55:36Z

src/fury/row/row.cc

+  int *start_from_lefts = new int[num_dims];
+  ArrayData **arrs = new ArrayData *[num_dims]; // root to current node


Why do you introduce two memory-leaked allocation here?

If you want to workaround VLA, you can use std::unique_ptr for this purpose.

GetDimensions can be removed, it's not used anymore

PragmaTwice · 2024-01-10T12:56:15Z

src/fury/meta/preprocessor.h

+#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


Could you explain these changes?

Because msvc will not expand the macro and will only pass it in as a parameter, this will lead to many syntax errors.

PragmaTwice · 2024-01-10T12:56:25Z

src/fury/meta/field_info.h

+#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


PragmaTwice · 2024-01-10T13:23:30Z

src/fury/meta/preprocessor.h

+#define FURY_PP_FOREACH_IMPL_2(X, _1, _2) X(_1), X(_2)
+#define FURY_PP_FOREACH_IMPL_3(X, _1, _2, _3) X(_1), X(_2), X(_3)
+#define FURY_PP_FOREACH_IMPL_4(X, _1, _2, _3, _4) X(_1), X(_2), X(_3), X(_4)
+#define FURY_PP_FOREACH_IMPL_5(X, _1, _2, _3, _4, _5) X(_1), X(_2), X(_3), X(_4), X(_5)
+#define FURY_PP_FOREACH_IMPL_6(X, _1, _2, _3, _4, _5, _6) X(_1), X(_2), X(_3), X(_4), X(_5), X(_6)
+#define FURY_PP_FOREACH_IMPL_7(X, _1, _2, _3, _4, _5, _6, _7) X(_1), X(_2), X(_3), X(_4), X(_5), X(_6), X(_7)


Is it necessary to change? It will reduce the generalization of this macro.

Compilation fails on C++ without modification, syntax error.

If there are some compile errors, could you figure out the root cause and search for a better solution?

You can refer to the implementation of Boost.Preprocessor.

Also, could you tell me the MSVC version you are using?

VS2022(14.38.33130)

src/fury/meta/preprocessor_test.cc(48): error C2146: 语法错误: 缺少“;”(在标识符“x”的前面) src/fury/meta/preprocessor_test.cc(49): error C2146: 语法错误: 缺少“;”(在标识符“x”的前面) src/fury/meta/preprocessor_test.cc(50): error C2146: 语法错误: 缺少“;”(在标识符“x”的前面) src/fury/meta/preprocessor_test.cc(51): error C2146: 语法错误: 缺少“;”(在标识符“x”的前面) src/fury/meta/preprocessor_test.cc(57): error C2607: 静态断言失败 src/fury/meta/preprocessor_test.cc(58): error C2131: 表达式的计算结果不是常数 src/fury/meta/preprocessor_test.cc(58): note: 超出范围的索引 1 导致失败；允许范围是 0 <= 索引 < 1 src/fury/meta/preprocessor_test.cc(59): error C2131: 表达式的计算结果不是常数 src/fury/meta/preprocessor_test.cc(59): note: 超出范围的索引 2 导致失败；允许范围是 0 <= 索引 < 1

This error occurs when calling FURY_PP_FOREACH.

It will reduce the generalization of this macro.

I don’t quite understand the problem you mentioned. The following code is also like this.

#define FURY_PP_NARG_REV() \ 63, 62, 61, 60, 59, 58, 57, 56, 55, 54, 53, 52, 51, 50, 49, 48, 47, 46, 45, \ 44, 43, 42, 41, 40, 39, 38, 37, 36, 35, 34, 33, 32, 31, 30, 29, 28, 27, \ 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, \ 8, 7, 6, 5, 4, 3, 2, 1, 0

Previously it does not need to be seperated by comma (,).

So that we can use it for other purpose.

So there is no problem with this modification here.

I cannot see any connection between it and FURY_PP_NARG_REV.

So there is no problem with this modification here.

Previously, this macro could be expanded in other ways, such as into symbols connected by a plus sign. Adding a comma makes it impossible.

I don't agree with this change. You'll need to look at Boost.Preprocessor to see more appropriate changes.

PragmaTwice · 2024-01-10T13:26:15Z

src/fury/encoder/row_encode_trait.h

+#ifndef __ROW_ENCODER_TRAIT_HEADER
+#define __ROW_ENCODER_TRAIT_HEADER


I think MSVC supports pragma once.

https://learn.microsoft.com/en-us/cpp/preprocessor/once?view=msvc-170

BTW, we just need to support the latest version of MSVC (VS2022, v143) to reduce maintanance effort.

If you are using an old version, please update it.

The msvc I use does not support #pragma once, and it is also VS2022(14.38.33130). Is it more versatile if we use this method?

https://learn.microsoft.com/en-us/cpp/preprocessor/pragma-directives-and-the-pragma-keyword?view=msvc-170

from this page, we can see that it is supported.

And also, here is a live example on godbolt: https://godbolt.org/z/xevh3Ejoj

Is it more versatile if we use this method?

It is not related to this PR. Feel free to open issue or a seperate PR for it.

PragmaTwice · 2024-01-10T15:06:49Z

Generally speaking, if supporting msvc cl makes the maintainability of the code worse, the code quality is significantly reduced, or the previous code structure has unreasonable changes, I would be inclined not to support msvc.

Users can use better compilers such as clang (or clang-cl with msvc) under windows.

LiangliangSui · 2024-01-10T15:19:09Z

msvc's support for C++ is different from gcc in many details (it will produce many strange compilation errors, you can try it locally). If you think that adapting msvc will reduce the maintainability of the code, we can completely ignore the windows compilation C++ without having to spend some unrewarded time. @PragmaTwice

PragmaTwice · 2024-01-10T15:41:27Z

I will look into the support of msvc and try it on my local these days, and provide my updates here.

LiangliangSui · 2024-01-10T15:57:31Z

I look forward to your ideas and hope to learn from you 💪

Move all default Serializer registrations to specific Serializers. Signed-off-by: LiangliangSui <coolsui.coding@gmail.com>

chaokunyang · 2024-11-02T16:09:54Z

Done in #1873

LiangliangSui requested review from PragmaTwice and chaokunyang as code owners January 10, 2024 12:29

PragmaTwice requested changes Jan 10, 2024

View reviewed changes

PragmaTwice reviewed Jan 10, 2024

View reviewed changes

chore(java): Move default Serializer registration location

f4571e4

Move all default Serializer registrations to specific Serializers. Signed-off-by: LiangliangSui <coolsui.coding@gmail.com>

chaokunyang closed this Nov 2, 2024

		int *start_from_lefts = new int[num_dims];
		ArrayData *arrs = new ArrayData [num_dims]; // root to current node

		#ifndef __ROW_ENCODER_TRAIT_HEADER
		#define __ROW_ENCODER_TRAIT_HEADER

Comments

Conversation

LiangliangSui commented Jan 10, 2024

Uh oh!

LiangliangSui commented Jan 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragmaTwice commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiangliangSui commented Jan 10, 2024

Uh oh!

PragmaTwice commented Jan 10, 2024

Uh oh!

LiangliangSui commented Jan 10, 2024

Uh oh!

chaokunyang commented Nov 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PragmaTwice Jan 10, 2024 •

edited

Loading

PragmaTwice Jan 10, 2024 •

edited

Loading

PragmaTwice Jan 10, 2024 •

edited

Loading

PragmaTwice Jan 10, 2024 •

edited

Loading

PragmaTwice commented Jan 10, 2024 •

edited

Loading