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.

Sign up for GitHub

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

Jump to bottom

[Fix] [Issue 338] Inconsistent id types #342

Merged
merged 8 commits into from
Sep 25, 2023
Merged

[Fix] [Issue 338] Inconsistent id types #342

merged 8 commits into from
Sep 25, 2023

Conversation

nrkramer
Copy link
Collaborator

@nrkramer nrkramer commented Sep 4, 2023 •
edited

Link to issue: #338

This PR fixes the inconsistent id issues throughout the library.

I also added explicit casts in many places to squelch compiler warnings coming from MSVC.

@ZigRazor This PR represents an API-breaking change and forces us to do a major release.

@nrkramer nrkramer self-assigned this Sep 4, 2023
@github-actions github-actions bot added test Something about test core something about core labels Sep 4, 2023
@ghost
Copy link

ghost commented Sep 4, 2023 •
edited by ghost

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

nrkramer
nrkramer commented Sep 4, 2023
View reviewed changes
include/Utility/id_t.hpp Outdated Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 4, 2023
View reviewed changes
include/Graph/Graph.hpp
@@ -1212,7 +1212,7 @@
}

unsigned int zippedBytes;
zippedBytes = gzwrite(outFileZ, content_ptr, strlen(content_ptr));
zippedBytes = gzwrite(outFileZ, content_ptr, (unsigned int)strlen(content_ptr));

Check notice

Code scanning / Flawfinder (reported by Codacy)

Does not handle strings that are not \0-terminated; if given one it may perform an over-read (it could cause a crash if unprotected) (CWE-126). Note

Does not handle strings that are not \0-terminated; if given one it may perform an over-read (it could cause a crash if unprotected) (CWE-126).
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 4, 2023
View reviewed changes
include/Utility/id_t.hpp Fixed Show fixed Hide fixed
include/Graph/Graph.hpp
@@ -3078,7 +3078,7 @@
while (true) {
// Go sequentially through buckets till one non-empty
// bucket is found
while (B[idx].size() == 0 && idx < maxWeight * V) {
while (B[idx].size() == 0u && idx < maxWeight * V) {

Check warning

Code scanning / Cppcheck (reported by Codacy)

Array index 'idx' is used before limits check. Warning

Array index 'idx' is used before limits check.
include/Graph/Graph.hpp
@@ -1212,7 +1212,7 @@
}

unsigned int zippedBytes;
zippedBytes = gzwrite(outFileZ, content_ptr, strlen(content_ptr));
zippedBytes = gzwrite(outFileZ, content_ptr, (unsigned int)strlen(content_ptr));

Check warning

Code scanning / Cppcheck (reported by Codacy)

Variable 'zippedBytes' is assigned a value that is never used. Warning

Variable 'zippedBytes' is assigned a value that is never used.
include/Partitioning/Partition.hpp Fixed Show resolved Hide resolved
@nrkramer
Copy link
Collaborator Author

nrkramer commented Sep 5, 2023 •
edited

Looks like the test for Tarjan's segfaults on master. Someone should bugfix that.

[----------] 7 tests from TarjanTest
[ RUN      ] TarjanTest.test_1
zsh: segmentation fault  ./test/test_exe

Also there seems to be some failure in Partitioning with releasing an unowned mutex on MSVC.

Both of these issues seem to be present on master, and are not caused by this PR.

@nrkramer
Copy link
Collaborator Author

nrkramer commented Sep 5, 2023

Codacity says the macro is unsafe because it can't resolve the type.

We should probably add an argument to the codacity action to ignore it (with -U) for CXXGRAPH_ID_TYPE

ZigRazor
ZigRazor requested changes Sep 5, 2023
View reviewed changes
include/Partitioning/CoordinatedPartitionState.hpp Outdated Show resolved Hide resolved
include/Partitioning/CoordinatedPartitionState.hpp Outdated Show resolved Hide resolved
include/Partitioning/CoordinatedRecord.hpp Show resolved Hide resolved
include/Partitioning/Partitioner.hpp Outdated Show resolved Hide resolved
include/Utility/id_t.hpp Outdated Show resolved Hide resolved
@ZigRazor
Copy link
Owner

ZigRazor commented Sep 5, 2023

Looks like the test for Tarjan's segfaults on master. Someone should bugfix that.

[----------] 7 tests from TarjanTest
[ RUN      ] TarjanTest.test_1
zsh: segmentation fault  ./test/test_exe

Also there seems to be some failure in Partitioning with releasing an unowned mutex on MSVC.

Both of these issues seem to be present on master, and are not caused by this PR.

This problem seems to be on Windows machine (MSVC), I think there is somenthings that does not work on STL implementation of MSVC, as for this pr #326

@ZigRazor
Copy link
Owner

ZigRazor commented Sep 5, 2023

have you the opportunity to try some other checks on this?

@nrkramer
Copy link
Collaborator Author

have you the opportunity to try some other checks on this?

Which other checks are you referring to? I ran the tests we have at-desk, and everything seems to pass (except for the Partition test due to the known unowned mutex bug)

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Merging #342 (0abdf9d) into master (7c03e1a) will decrease coverage by 0.02%.
Report is 2 commits behind head on master.
The diff coverage is 89.42%.

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
- Coverage   97.31%   97.30%   -0.02%     
==========================================
  Files          55       55              
  Lines        8455     8455              
==========================================
- Hits         8228     8227       -1     
- Misses        227      228       +1
Flag Coverage Δ
unittests 97.30% <89.42%> (-0.02%) ⬇️

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

Files Changed Coverage Δ
include/Partitioning/CoordinatedPartitionState.hpp 75.63% <0.00%> (ø)
include/Partitioning/CoordinatedRecord.hpp 73.80% <0.00%> (ø)
include/Utility/Typedef.hpp 100.00% <ø> (ø)
include/Graph/Graph.hpp 94.86% <95.45%> (ø)
include/Edge/DirectedEdge.hpp 100.00% <100.00%> (ø)
include/Edge/DirectedWeightedEdge.hpp 100.00% <100.00%> (ø)
include/Edge/Edge.hpp 100.00% <100.00%> (ø)
include/Edge/UndirectedEdge.hpp 100.00% <100.00%> (ø)
include/Edge/UndirectedWeightedEdge.hpp 100.00% <100.00%> (ø)
include/Node/Node.hpp 100.00% <100.00%> (ø)
... and 12 more

... and 1 file with indirect coverage changes

@nrkramer
Copy link
Collaborator Author

This PR seems to be in a pretty good state now.

There's no build warnings on either mac or Windows, which is a breath of fresh air.

@ZigRazor ZigRazor merged commit 8f86225 into master Sep 25, 2023
16 of 19 checks passed
@ZigRazor ZigRazor linked an issue Sep 25, 2023 that may be closed by this pull request
Inconsistent id types #338
Closed
@ZigRazor ZigRazor mentioned this pull request Sep 28, 2023
Merged
@ZigRazor ZigRazor deleted the issue/338 branch January 9, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZigRazor ZigRazor ZigRazor approved these changes

@sbaldu sbaldu Awaiting requested review from sbaldu

Assignees

@nrkramer nrkramer

Labels
core something about core test Something about test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent id types
2 participants
@nrkramer @ZigRazor