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

Fixing warnings during building related to nutaudet folder, about unused or uninitialized variables #481

Merged
merged 13 commits into from
Oct 24, 2024

Conversation

antonioiuliano2
Copy link
Contributor

Dear all FairShip users,

This commit fixes all nutaudet warnings happening during building the code.

They were about either unused or uninitialized variables: most of the times the warnings were genuine, i.e. the variable was actually "forgotten" in the code and never used. In other instances, however, the object was actually used, even if the variable was not. This happens a lot with TGeoShapes, since we define the variable when we build the shape, but we do not use the variable itself in the code. I fixed these warnings by addind a redundant SetName call after building the shape.

I attach the initial log with the warnings, as provided by @olantwin, for reference. I also attach the new log, without nutaudet warnings.

I will fix the charmdet warnings in another branch, since they are related to a completely different geometry.

Best Regards,
Antonio

FairShip_Oliver.log
fixingnutaudetwarnings_build.log

Copy link
Contributor

@olantwin olantwin left a comment

Choose a reason for hiding this comment

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

First of all, thanks!

Some remarks:

  • For unused variables that are needed (e.g. the different shapes/volumes): Please use the [[maybe_unused]] attribute instead of adding unnecessary statements.
  • I don't think we need to make these fixes for the charmdet dir, as I believe they are outdated relative to the muonflux branch anyway. I would propose removing charmdet from the master branch instead.

nutaudet/Target.cxx Outdated Show resolved Hide resolved
nutaudet/Target.cxx Outdated Show resolved Hide resolved
nutaudet/Target.cxx Outdated Show resolved Hide resolved
nutaudet/Target.cxx Outdated Show resolved Hide resolved
nutaudet/NuTauMudet.cxx Outdated Show resolved Hide resolved
@antonioiuliano2 antonioiuliano2 requested a review from olantwin July 31, 2024 12:28
Copy link
Contributor

@olantwin olantwin left a comment

Choose a reason for hiding this comment

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

Nearly perfect.

Could you please also add a line to the change log?

nutaudet/EmulsionMagnet.cxx Outdated Show resolved Hide resolved
nutaudet/EmulsionMagnet.cxx Outdated Show resolved Hide resolved
nutaudet/EmulsionMagnet.cxx Outdated Show resolved Hide resolved
nutaudet/EmulsionMagnet.cxx Outdated Show resolved Hide resolved
nutaudet/NuTauMudet.cxx Outdated Show resolved Hide resolved
nutaudet/Target.cxx Outdated Show resolved Hide resolved
nutaudet/Target.cxx Outdated Show resolved Hide resolved
nutaudet/Target.cxx Outdated Show resolved Hide resolved
@antonioiuliano2 antonioiuliano2 requested a review from a team as a code owner September 16, 2024 10:49
Copy link
Contributor

@olantwin olantwin left a comment

Choose a reason for hiding this comment

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

See comment, everything else looks good to me now.

Before the final merge, you'd have to rebase to deal with the conflict in the CHANGELOG.md.

nutaudet/Target.cxx Outdated Show resolved Hide resolved
@antonioiuliano2
Copy link
Contributor Author

antonioiuliano2 commented Sep 17, 2024

See comment, everything else looks good to me now.

Before the final merge, you'd have to rebase to deal with the conflict in the CHANGELOG.md.

Done, I had to force push afterwards, because git complained about the changed history.

@olantwin
Copy link
Contributor

Done, I had to force push afterwards, because git complained about the changed history.

In general, you'll have to do a force push after a rebase. Next time, please do it as the last thing, once everything else is resolved, as it makes it harder to review the changes.

This time it's fine though, as there were very few outstanding issues.

Copy link
Contributor

@olantwin olantwin left a comment

Choose a reason for hiding this comment

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

99% perfect ;)

Sorry for being so nitpicky!

nutaudet/NuTauMudet.cxx Outdated Show resolved Hide resolved
nutaudet/Target.cxx Outdated Show resolved Hide resolved
nutaudet/HPT.cxx Outdated Show resolved Hide resolved
nutaudet/HPT.cxx Outdated Show resolved Hide resolved
@olantwin
Copy link
Contributor

olantwin commented Oct 3, 2024

@antonioiuliano2 Can you please give an estimate of when the requested changes can be implemented? (or let me know if you disagree with any comments?)

This PR has now been open for 3 months.

Thanks!

@antonioiuliano2
Copy link
Contributor Author

Sorry, I finally got back to this pull request, I have checked all the remaining comments

@olantwin
Copy link
Contributor

Sorry, I finally got back to this pull request, I have checked all the remaining comments

Thanks a lot!

Copy link
Contributor

@olantwin olantwin left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this cleanup, and the patience with my suggestions!

@olantwin olantwin merged commit 62496f4 into ShipSoft:master Oct 24, 2024
1 check failed
@antonioiuliano2 antonioiuliano2 deleted the nutaudet_warning_fixes branch October 24, 2024 13:34
anupama-reghunath pushed a commit to anupama-reghunath/FairShip that referenced this pull request Nov 11, 2024
…sed or uninitialized variables (ShipSoft#481)

* fixing warnings during building, about unused or uninitialized variables

* labeling not directly used volume variables with maybe unused

* replacing volTungsten and volLead with a single volAbsorber variable

* do not repeat name of the TGeoBBox pointer

* using auto* to avoid repeating type, fixing whitespaces

* ternary operator instead of if else, small indentation fixes

* adding pull request info to CHANGELOG

* using same names for lead and tungsten volumes

* boolean initialization to false instead of 0

* Update nutaudet/Target.cxx

Yes, that is great, thanks!

Co-authored-by: Oliver Lantwin <oliver.lantwin@cern.ch>

* setname of shape immediately for easier readability

* replacing C-style cast with static cast

* set TGeoBBox name directly at object creation

---------

Co-authored-by: Oliver Lantwin <oliver.lantwin@cern.ch>
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.

2 participants