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

Add option for stricter compiler warnings #1891

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Add option for stricter compiler warnings #1891

wants to merge 6 commits into from

Conversation

fabian-lunarg
Copy link
Contributor

@fabian-lunarg fabian-lunarg commented Nov 27, 2024 •
edited
Loading

  • Add a cmake-option: raise compiler-warnings level for GCC and clang
  • apply stricter settings for targets gfxrecon_format, gfxrecon_util, gfxrecon_graphics

first step towards #1890

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 311184.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5413 running.

@fabian-lunarg fabian-lunarg force-pushed the fabian_stricter_compiler_warnings_guarded branch from 0bda28c to 039a7dc Compare November 27, 2024 12:22
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 311214.

@fabian-lunarg fabian-lunarg force-pushed the fabian_stricter_compiler_warnings_guarded branch from 039a7dc to 8bf21c2 Compare November 27, 2024 12:38
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 311244.

@fabian-lunarg fabian-lunarg self-assigned this Nov 27, 2024
@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5416 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5416 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 311459.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5420 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5420 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 311489.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5421 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5421 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 311512.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5422 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5422 passed.

@fabian-lunarg fabian-lunarg force-pushed the fabian_stricter_compiler_warnings_guarded branch from 85fe2eb to 3729798 Compare November 28, 2024 07:49
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 312150.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5439 running.

panos-lunarg
panos-lunarg reviewed Nov 28, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5439 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 312194.

@fabian-lunarg fabian-lunarg force-pushed the fabian_stricter_compiler_warnings_guarded branch from 0dde75d to 7ee61b8 Compare November 28, 2024 09:07
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 312200.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 312202.

@fabian-lunarg fabian-lunarg force-pushed the fabian_stricter_compiler_warnings_guarded branch from 77884ec to 8b843d5 Compare November 28, 2024 10:51
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 312279.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5446 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5446 passed.

@bradgrantham-lunarg
Copy link
Contributor

@jzulauf-lunarg can you review this to make sure it meets your expectations for stricter code? The CMake changes are a nice bonus but mostly with this options do these changes make sense to you?

@jzulauf-lunarg
Copy link
Contributor

Looks like good work. My preference would be that if we tighten the compiler warnings, once the issues are fixed that the tightened warnings become the default, not an option. Otherwise you have to rework commits from anyone not enabling the option.

Also, to the extent possible, any stricter setting should be reflected across all platforms. As I primarily develop on windows, I'm continually having to touch up changes when build testing against Linux, or errors from CI.

jzulauf-lunarg
jzulauf-lunarg reviewed Dec 3, 2024
View reviewed changes
Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

A few question re: the code fixes.

framework/decode/file_processor.h Outdated Show resolved Hide resolved
framework/decode/file_processor.h Outdated Show resolved Hide resolved
framework/decode/file_processor.h Outdated Show resolved Hide resolved
framework/generated/generated_vulkan_struct_deep_copy_stype.cpp Show resolved Hide resolved
framework/graphics/fps_info.cpp Show resolved Hide resolved
framework/graphics/fps_info.h Show resolved Hide resolved
framework/graphics/vulkan_resources_util.cpp Show resolved Hide resolved
framework/util/image_writer.cpp Show resolved Hide resolved
framework/util/image_writer.cpp Show resolved Hide resolved
framework/util/image_writer.cpp Outdated Show resolved Hide resolved
@fabian-lunarg
Copy link
Contributor Author

fabian-lunarg commented Dec 3, 2024 •
edited
Loading

Looks like good work. My preference would be that if we tighten the compiler warnings, once the issues are fixed that the tightened warnings become the default, not an option. Otherwise you have to rework commits from anyone not enabling the option.

Also, to the extent possible, any stricter setting should be reflected across all platforms. As I primarily develop on windows, I'm continually having to touch up changes when build testing against Linux, or errors from CI.

many thanks @jzulauf-lunarg!

yes, absolutely right, we want to enforce those settings as defaults and windows is still missing entirely (as is android).
this goal is tracked by the parent-issue #1890
the cmake-option is (meant to be) only temporary and part of the proposed transition-strategy.

@fabian-lunarg fabian-lunarg force-pushed the fabian_stricter_compiler_warnings_guarded branch from 8b843d5 to 7767c1b Compare December 3, 2024 08:31
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 315471.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5484 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5484 passed.

jzulauf-lunarg
jzulauf-lunarg approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM

@bradgrantham-lunarg bradgrantham-lunarg mentioned this pull request Dec 4, 2024
Draft
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 317038.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5514 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5514 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panos-lunarg panos-lunarg panos-lunarg left review comments

@antonio-lunarg antonio-lunarg antonio-lunarg approved these changes

@bradgrantham-lunarg bradgrantham-lunarg bradgrantham-lunarg approved these changes

@jzulauf-lunarg jzulauf-lunarg jzulauf-lunarg approved these changes

Assignees

@fabian-lunarg fabian-lunarg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fabian-lunarg @ci-tester-lunarg @bradgrantham-lunarg @jzulauf-lunarg @panos-lunarg @antonio-lunarg