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

Fix Compile Warnings #5963

Merged
merged 39 commits into from
Jul 29, 2024
Merged

Conversation

Ocraftyone
Copy link
Contributor

Description

This PR clears up almost all of the compile warnings generated when building the slicer on Windows. There are still a few signed/unsigned issues, but fixing those would require larger codebase changes. I tried to keep these changes pretty simple.

Tests

Tested on Windows 11

At least as much as possible without significantly altering parts of the application
json::parse was being called on the object, rather than statically like it should be. Also, the value was not being captured.
By having the definition in the header, it causes issues when other files define ICON_SIZE. By renaming it to MM_ICON_SIZE, this lessens the issue. It would probably be ideal to have the definitions in the respective .cpp that use them, but it would make it less convenient to update the values if needed in the future.
@vovodroid
Copy link
Contributor

With warnings you can at least to see build progress)))

@Ocraftyone Ocraftyone changed the title Fix Windows Compile Warnings Fix Compile Warnings Jul 5, 2024
Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

Thank you so much for your effort!

@SoftFever SoftFever merged commit b83e16d into SoftFever:main Jul 29, 2024
9 of 14 checks passed
SoftFever added a commit that referenced this pull request Jul 29, 2024
SoftFever added a commit that referenced this pull request Aug 11, 2024
This reverts commit b83e16d.

Found regressions like auto orientation didn't work anymore after this change, revert it
@SoftFever
Copy link
Owner

@Ocraftyone

I wanted to give a heads-up that I reverted this PR due to some issues I found.
It's a bit costly to test and go through all the changes so I decided to revert the change instead.

Issues I found in case you're curious:

  • Auto orientation is no longer working as expected.
  • There are unintended consequences in src/libslic3r/Arrange.cpp.
  • A function call was removed due to a warning about unused return values, but it's still necessary for the code to function correctly.

@Ocraftyone
Copy link
Contributor Author

No problem! I wasn't entirely done with development on this anyway, so I'll continue to work on it when I can 😁

@vovodroid
Copy link
Contributor

Probably let's split the task - for instance I guess it's harmless to resolve unreferenced local variable warning.

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.

3 participants