feat(C++): Enable Windows C++ CI and fix some compilation errors #1326
Conversation
LiangliangSui
requested review from
chaokunyang and
PragmaTwice
as code owners
|
Windows C++ CI has passed. PTAL! |
src/fury/thirdparty/MurmurHash3.cc
Outdated
| #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) |
There was a problem hiding this comment.
Could you explain this change? I don't think it need to be changed.
There was a problem hiding this comment.
Sorry, a missing header file caused a compilation error. I will modify it back.
src/fury/row/row.cc
Outdated
| int *start_from_lefts = new int[num_dims]; | ||
| ArrayData **arrs = new ArrayData *[num_dims]; // root to current node |
There was a problem hiding this comment.
Why do you introduce two memory-leaked allocation here?
There was a problem hiding this comment.
If you want to workaround VLA, you can use std::unique_ptr for this purpose.
There was a problem hiding this comment.
GetDimensions can be removed, it's not used anymore
src/fury/meta/preprocessor.h
Outdated
| #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 |
There was a problem hiding this comment.
Could you explain these changes?
There was a problem hiding this comment.
Because msvc will not expand the macro and will only pass it in as a parameter, this will lead to many syntax errors.
src/fury/meta/field_info.h
Outdated
| #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 |
src/fury/meta/preprocessor.h
Outdated
| #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) |
There was a problem hiding this comment.
Is it necessary to change? It will reduce the generalization of this macro.
There was a problem hiding this comment.
Compilation fails on C++ without modification, syntax error.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
VS2022(14.38.33130)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Previously it does not need to be seperated by comma (,).
So that we can use it for other purpose.
There was a problem hiding this comment.
So there is no problem with this modification here.
There was a problem hiding this comment.
I cannot see any connection between it and FURY_PP_NARG_REV.
There was a problem hiding this comment.
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.
src/fury/encoder/row_encode_trait.h
Outdated
| #ifndef __ROW_ENCODER_TRAIT_HEADER | ||
| #define __ROW_ENCODER_TRAIT_HEADER |
There was a problem hiding this comment.
I think MSVC supports pragma once.
https://learn.microsoft.com/en-us/cpp/preprocessor/once?view=msvc-170
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
from this page, we can see that it is supported.
There was a problem hiding this comment.
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.
|
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. |
|
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 |
|
I will look into the support of msvc and try it on my local these days, and provide my updates here. |
|
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>
Use
windows-2022in CI and fix some C++ compilation errors in windows.