Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows builds are broken due to MSVC fold expression bug #416

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

nolankramer
Copy link
Collaborator

This is addressed by adding workaround type traits into TypeTraits.hpp:

template<typename T, typename... Ts>
struct all_are_node_ptrs {
    static constexpr bool value = (is_node_ptr_v<T> && ... && is_node_ptr_v<Ts>);
};

template<typename T, typename... Ts>
inline constexpr bool all_are_node_ptrs_v = all_are_node_ptrs<T, Ts...>::value;

and

template<typename T, typename... Ts>
struct all_are_edge_ptrs {
    static constexpr bool value = (is_edge_ptr_v<T> && ... && is_edge_ptr_v<Ts>);
};

template<typename T, typename... Ts>
inline constexpr bool all_are_edge_ptrs_v = all_are_edge_ptrs<T, Ts...>::value;

I've also cleaned-up warnings on Windows that were caused by not explicitly typecasting.

@sbaldu
Copy link
Collaborator

sbaldu commented Apr 22, 2024

The clang-format workflow fails in files test/DijkstraTest.cpp, TypeTraitsTest.hpp and welshPowellColoringTest.cpp.
I just noticed, also some files in include.

@nolankramer
Copy link
Collaborator Author

@sbaldu Ran it. Expect a high number of changes! Seems quite a few files weren't formatted correctly.

@nolankramer
Copy link
Collaborator Author

Looks like format is still broken on include/???

I wonder why. I'm using a freshly installed clang-format on WSL Ubuntu: Ubuntu clang-format version 14.0.0-1ubuntu1.1

@sbaldu
Copy link
Collaborator

sbaldu commented Apr 22, 2024

The workflow uses clang-format 16, maybe it's due to some difference in the two versions. It's happened to me before when working with clang-format 14

@sbaldu sbaldu self-requested a review April 22, 2024 21:18
@nolankramer
Copy link
Collaborator Author

Hmm let me try to get that version

@sbaldu
Copy link
Collaborator

sbaldu commented Apr 22, 2024

@nolankramer I see that partition test is failing on windows, but I suppose that the issue is unrelated to this PR.

Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 96.21849% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 97.88%. Comparing base (1b31e63) to head (717f833).
Report is 21 commits behind head on master.

Files Patch % Lines
include/CXXGraph/Graph/Algorithm/Dijkstra_impl.hpp 95.48% 8 Missing ⚠️
include/CXXGraph/Graph/Algorithm/Boruvka_impl.hpp 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
+ Coverage   97.58%   97.88%   +0.29%     
==========================================
  Files          87       87              
  Lines        9492    10057     +565     
  Branches        0      666     +666     
==========================================
+ Hits         9263     9844     +581     
+ Misses        229      213      -16     
Flag Coverage Δ
unittests 97.88% <96.21%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nolankramer
Copy link
Collaborator Author

nolankramer commented Apr 22, 2024

@nolankramer I see that partition test is failing on windows, but I suppose that the issue is unrelated to this PR.

FWIW, that test has been failing for a long time. Somebody is grabbing a mutex they don't own in the partitioning code. That can likely be cleaned up in another PR.

@sbaldu
Copy link
Collaborator

sbaldu commented Apr 22, 2024

@nolankramer I see that partition test is failing on windows, but I suppose that the issue is unrelated to this PR.

FWIW, that test has been failing for a long time. Somebody is grabbing a mutex they don't own in the partitioning code. That can likely be cleaned up in another PR.

Yes you're right.

@sbaldu
Copy link
Collaborator

sbaldu commented Apr 22, 2024

@nolankramer Perfect

@nolankramer
Copy link
Collaborator Author

nolankramer commented Apr 22, 2024

This can be merged after checks. Looks like the failing ones are due to infra bugs (and the tests b/c of the partitioning bug).

@nolankramer
Copy link
Collaborator Author

@sbaldu This can be merged now.

@sbaldu
Copy link
Collaborator

sbaldu commented Apr 22, 2024

I agree. I'm waiting for @ZigRazor approval.

Copy link
Owner

@ZigRazor ZigRazor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one!

@ZigRazor
Copy link
Owner

We can merge it

@ZigRazor ZigRazor linked an issue Apr 22, 2024 that may be closed by this pull request
@ZigRazor ZigRazor added the core something about core label Apr 22, 2024
@ZigRazor ZigRazor self-assigned this Apr 22, 2024
@ZigRazor ZigRazor merged commit 11e80f2 into ZigRazor:master Apr 22, 2024
28 of 32 checks passed
@ZigRazor
Copy link
Owner

I think we can open a new issue for partitioning test failed, that you cite before @nolankramer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core something about core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows builds are broken due to MSVC fold expression bug
3 participants