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 exception parameter not being used #194

Merged
merged 3 commits into from
Mar 10, 2023
Merged

Fix exception parameter not being used #194

merged 3 commits into from
Mar 10, 2023

Conversation

achim-k
Copy link
Collaborator

@achim-k achim-k commented Mar 10, 2023

Public-Facing Changes

None

Description

  • Enables stricter clang compilation flags
  • Fix unused exception parameter

fixes #193

@achim-k achim-k requested a review from jtbandes March 10, 2023 15:02
jtbandes
jtbandes approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

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

Personally, my strategy with clang is usually to start with -Weverything and disable the annoying warnings.

CMakeLists.txt
@@ -17,7 +17,10 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
macro(enable_strict_compiler_warnings target)
target_compile_options(${target} PRIVATE
$<$<CXX_COMPILER_ID:MSVC>:/WX /W4>
$<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall -Wextra -Wpedantic -Werror -Wold-style-cast -Wfloat-equal>
Copy link
Member

Choose a reason for hiding this comment

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

This conditional stuff is crazy – can we not just use regular if() to do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can change it to that, yes

@achim-k
Copy link
Collaborator Author

achim-k commented Mar 10, 2023 •
edited
Loading

Personally, my strategy with clang is usually to start with -Weverything and disable the annoying warnings.

Had this at the beginning, but raised to many compile warnings. We should have done this from the start

Edit: Just checked again, maybe I can get around this quite fast. Seems like most errors are due to -Wc++98-compat
Edit2: That resolves most of the warnings but there are still plenty of others (-Wweak-template-vtables, -Wweak-tables, -Wcovered-switch-default, -Wpadded, ...)

@achim-k achim-k mentioned this pull request Mar 10, 2023
Open
@achim-k achim-k merged commit c99ef97 into main Mar 10, 2023
@achim-k achim-k deleted the achim/unused_exception_parameter branch March 10, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtbandes jtbandes jtbandes approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unused exception variable warning not detected by CI
2 participants
@achim-k @jtbandes