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

[CURA-11359] sanitize code #1986

Merged
merged 50 commits into from
Dec 1, 2023
Merged

[CURA-11359] sanitize code #1986

merged 50 commits into from
Dec 1, 2023

Conversation

wawanbreton
Copy link
Contributor

@wawanbreton wawanbreton commented Nov 22, 2023

The main purpose of this PR is to remove all the compilation warnings issued when using the enable_extensive_warnings option is set. On the currant master branch, there are 16000+ warnings which makes it completely impossible to use them relevantly.

Most notable changes made:

  • Renaming of geometry classes to make sure that the used variables types are clear and consistent all over the code. This helps removing the warnings.
  • Renaming of most members variables with a _ as suffix to diferentiate them from local variables
  • Removed some dead classes (ProximityPointLink, ClientSocket).
  • Updated spdlog/fmt versions to suppress some warnings coming from them. This comes wih the same update for synsepalum-dulcificum and also for gradual flow plugin

Some parts of the code have become less readable, especially because of the numbers conversions. This emphasizes a global code issue that various types are used and mixed together, which should not happen. This should be fixed along the way.

@wawanbreton wawanbreton marked this pull request as draft November 22, 2023 09:40
Copy link
Contributor

github-actions bot commented Nov 22, 2023

Unit Test Results

26 tests  ±0   26 ✔️ ±0   6s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit fad6bd5. ± Comparison against base commit 537f1c2.

♻️ This comment has been updated with latest results.

@wawanbreton wawanbreton marked this pull request as ready for review November 23, 2023 08:53
Comment on lines +144 to +152
- name: Upload the detailed tests report
uses: actions/upload-artifact@v3
if: ${{ always() }}
with:
name: LastTest.log
path: |
build/Release/Testing/Temporary/LastTest.log
retention-days: 5

Copy link
Contributor

Choose a reason for hiding this comment

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

should this be part of this PR? seems seperated to me

can it be removed?

Suggested change
- name: Upload the detailed tests report
uses: actions/upload-artifact@v3
if: ${{ always() }}
with:
name: LastTest.log
path: |
build/Release/Testing/Temporary/LastTest.log
retention-days: 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.. I thought this could be part of it, otherwise it would be lost. Should I make a mini-PR for it ?

include/slicer.h Show resolved Hide resolved
src/utils/PolygonsPointIndex.cpp Show resolved Hide resolved
src/utils/ExtrusionLine.cpp Outdated Show resolved Hide resolved
src/bridge.cpp Outdated Show resolved Hide resolved
src/bridge.cpp Outdated Show resolved Hide resolved
@casperlamboo
Copy link
Contributor

Also, there are some merge conflicts

@casperlamboo casperlamboo merged commit 7be00de into main Dec 1, 2023
16 of 20 checks passed
@casperlamboo casperlamboo deleted the CURA-11359_sanitize_code branch December 1, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants