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

COMP: fixed various Wunused-but-set-variable warnings from Clang 13 #2778

Merged
merged 1 commit into from
Oct 25, 2021
Merged

COMP: fixed various Wunused-but-set-variable warnings from Clang 13 #2778

merged 1 commit into from
Oct 25, 2021

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Oct 4, 2021

Three solutions:

  • removed the dead code.
  • use the variable by logging it
  • cast variable to void

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@github-actions github-actions bot added area:Core Issues affecting the Core module area:IO Issues affecting the IO module area:Segmentation Issues affecting the Segmentation module type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Oct 4, 2021
@seanm
Copy link
Contributor Author

seanm commented Oct 4, 2021

@thewtex @dzenanz @hjmjohnson I'm not at all sure these are the best fixes here. But figured I'd do something and we can discuss here.

Only someone that knows these classes can really know if the code is dead from an old refactoring, or if those variables should in fact be used.

dzenanz
dzenanz reviewed Oct 4, 2021
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

I made some suggestions where I felt different from your proposal.

Modules/Core/Common/test/itkPeriodicBoundaryConditionTest.cxx Show resolved Hide resolved
Modules/Core/Common/test/itkPeriodicBoundaryConditionTest.cxx Outdated Show resolved Hide resolved
Modules/IO/MRC/src/itkMRCHeaderObject.cxx Show resolved Hide resolved
@seanm seanm force-pushed the Wunused-but-set-variable branch 2 times, most recently from 8d84585 to 58a7f48 Compare October 12, 2021 17:00
@dzenanz
Copy link
Member

dzenanz commented Oct 12, 2021

Modules/IO/MRC/src/itkMRCHeaderObject.cxx:449:0: error: unterminated argument list invoking macro "itkWarningMacro" and Modules/IO/MRC/src/itkMRCHeaderObject.cxx:159:3: error: 'itkWarningMacro' was not declared in this scope should be fixed.

@seanm seanm force-pushed the Wunused-but-set-variable branch from 58a7f48 to a79936d Compare October 15, 2021 16:51
@kwrobot-v1
Copy link

kwrobot-v1 bot commented Oct 15, 2021

Errors:

  • Failed to reserve ref a79936d for the merge request: invalid git ref: 'no such commit'.
  • Failed to run the checks: mr utilities error: failed to list commits of a79936ddf0c04d09e5cb36f9a961c38f535fbeb6 for https://github.com/InsightSoftwareConsortium/ITK/pull/2778: fatal: bad object a79936ddf0c04d09e5cb36f9a961c38f535fbeb6 .

@seanm
COMP: fixed various Wunused-but-set-variable warnings from Clang 13
8a86299 
Two solutions:
- removed the dead code.
- use the variable by logging it
@seanm seanm force-pushed the Wunused-but-set-variable branch from a79936d to 8a86299 Compare October 21, 2021 18:49
@seanm
Copy link
Contributor Author

seanm commented Oct 25, 2021

So is this patch good to go?

The 2 failed checks are both Error renaming from "InsightToolkit-5.2.0/.ExternalData/MD5" to "/home/vsts/work/1/s/.ExternalData/MD5": No such file or directory which I assume is unrealted to this patch...

@jhlegarreta jhlegarreta self-requested a review October 25, 2021 20:51
@jhlegarreta
Copy link
Member

As @seanm says, the failures seem unrelated. Please @thewtex, have a look at the patch set 👀. Thanks.

@dzenanz dzenanz merged commit b85f246 into InsightSoftwareConsortium:master Oct 25, 2021
@seanm
Copy link
Contributor Author

seanm commented Oct 25, 2021

Alright! That should fix the last of the non-HDF warnings from my bots!

blowekamp
blowekamp reviewed Oct 26, 2021
View reviewed changes
Modules/Core/Common/test/itkPeriodicBoundaryConditionTest.cxx
}
++testIter;
}
std::cout << "Dummy sum: " << sum << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

@seanm
Were are not getting some valgrind defect related to this change:
https://open.cdash.org/viewDynamicAnalysisFile.php?id=8854676

It looks like the image buffer is never initialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look, and try with ASan too...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alas, ASan reports nothing, which is expected if it's really uninitialized memory use. But looking over the code, I don't spot the issue...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've run the test many times and always get the same number, -3.36317e+12. Is that what you get? I'd expect different results if it's using uninitialized memory.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't run the test. I just saw that the buffer was uninitialized.

I have had uninitialized values be reproducible on a system before. Do you get the same for debug and release? different OS? Can you tell me where the pixel values come from?

Copy link
Member

Choose a reason for hiding this comment

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

I was getting 0 for the value. I made what I expect to be the fix in PR #2841

@N-Dekker N-Dekker mentioned this pull request Apr 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blowekamp blowekamp blowekamp left review comments

@jhlegarreta jhlegarreta jhlegarreta approved these changes

@dzenanz dzenanz dzenanz approved these changes

Assignees
No one assigned
Labels
area:Core Issues affecting the Core module area:IO Issues affecting the IO module area:Segmentation Issues affecting the Segmentation module type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seanm @dzenanz @jhlegarreta @blowekamp