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

basic: fix more unused-but-set variables #1811

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

lakshmih
Copy link
Contributor

Removed the num_printed variable and replaced asserts on status with error-checking

svenvh
svenvh previously approved these changes Sep 18, 2023
EwanC
EwanC previously approved these changes Sep 19, 2023
@lakshmih
Copy link
Contributor Author

lakshmih commented Sep 19, 2023

@bashbaug can this be merged since it has two approvals? Not having the fix is breaking builds for us.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

I think this change is generally OK, but just to be sure I understand: the issue is variables that are assigned to but that are only used in asserts, which triggers an unused variable warning, which causes a build failure due to warnings-as-errors?

It's a little unfortunate to lose otherwise useful asserts, but using asserts like this isn't best practice in the CTS tests, so I mostly just want to understand exactly what's happening.

I also have one specific question, see below. Thanks!

test_conformance/basic/test_progvar.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Thanks!

I'm fine merging this as-is especially if unblocks you, but I was curious how the "Determine the max global id" code was used because it doesn't seem to be determining the maximum global ID, and it turns out it's dead code and l_max_global_id0 is never used. If we don't want to remove it in this PR, could we remove it in a subsequent PR, or at least file an issue so we don't forget to clean this up in the future?

@lakshmih
Copy link
Contributor Author

Ben, can we merge this, I'll push a follow up change.

@bashbaug bashbaug merged commit 7759c26 into KhronosGroup:main Sep 21, 2023
6 checks passed
@lakshmih lakshmih deleted the basic_compilation_fix branch September 21, 2023 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants