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 some compiler warnings #10

Merged
merged 4 commits into from
Jul 1, 2024
Merged

Fix some compiler warnings #10

merged 4 commits into from
Jul 1, 2024

Conversation

jmcarcell
Copy link
Contributor

@jmcarcell jmcarcell commented Jul 15, 2023

BEGINRELEASENOTES

  • Fix warnings about not passing exceptions by reference
  • Fix warnings about an implicit copy constructor defined
  • Fix other warnings, about fallthroughs, a weird UTF-8 character (spanish accent in comments in spanish) and unused results

ENDRELEASENOTES

The fallthroughs I'm not sure if they are intended but I just left the current behavior...

src/test/testgear.cc Outdated Show resolved Hide resolved
@tmadlener tmadlener mentioned this pull request Jul 17, 2023
@tmadlener
Copy link
Contributor

@scott-snyder
Copy link

The unused result warnings arise due to compiling with -DNDEBUG suppressing the assertions. The changes here will break compilation if assertions are enabled. It's probably better to add #undef NDEBUG to the start of the test source files so that we're actually testing what was intended.

@jmcarcell
Copy link
Contributor Author

More warings have been fixed now and debug builds should work again.

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Since this should only have little actual usage, I am fine with these changes and would merge them, unless you still want to changes some things.

@jmcarcell
Copy link
Contributor Author

I think these should be fine, the cmake changes behave like they did before and for the rest they seem quite harmless but I haven't tested them because there aren't any tests. At least it builds and Marlin keeps working.

@tmadlener tmadlener merged commit a1246f6 into iLCSoft:master Jul 1, 2024
4 checks passed
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.

None yet

3 participants