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

Move assignments outside of if statement #18

Conversation

tmadlener
Copy link
Contributor

Starting from version 13, gcc starts warning about this construct only if it's negated and when building with -std=c++20. No warning is raised with -std=c++17 or when using gcc12 (see mini demonstrator no compiler explorer with gcc12 and gcc13).

Explicitly lifting out the result of the assignment into a variable and checking that explicitly gets around this.

I will try to figure out whether the fact, that this works for if ((a = b)), but not for if (!(a = b)) is a gcc bug or expected behavior.

This is, I think the better way to fix what I attempted in #17

@tmadlener
Copy link
Contributor Author

I have submitted this thread to the gcc-help mailing list.

if ((!(a = b)))

is the correct way of wrapping this in parentheses to silence the warning as explained in the thread. Also this is not a false positive, the main question at this point is why the warning is only emitted with c++20.

Copy link
Collaborator

@AndyChappell AndyChappell left a comment

Choose a reason for hiding this comment

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

Hi Thomas, thanks for the proposed changes. I think my preference here would actually be a change of style. As these are PandoraInputTypes the = operator is overloaded here (see this link, so the effective logic here is

m_chi2 = chi2;
if (m_chi2.IsInitialized())
   ...

In this case, I think this approach, here and in the other examples probably serves to both address the issue and add some clarity.

Move assignements outside of the if statments and check for
initialization only in if statements
@tmadlener tmadlener force-pushed the fix-parentheses-error branch from 0f686e8 to 90a937d Compare December 13, 2024 07:36
@tmadlener
Copy link
Contributor Author

Done.

Copy link
Collaborator

@AndyChappell AndyChappell left a comment

Choose a reason for hiding this comment

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

Changes look good. Thanks.

@AndyChappell
Copy link
Collaborator

@tmadlener The proposed changes look good. I will aim to integrate these into a release candidate early in the new year.

@AndyChappell AndyChappell changed the base branch from master to feature/gcc13_support January 6, 2025 11:57
@AndyChappell AndyChappell merged commit cf7de5f into PandoraPFA:feature/gcc13_support Jan 6, 2025
@AndyChappell
Copy link
Collaborator

@tmadlener All of the changes (plus some additional admin) have now been merged across all three relevant repositories.

@tmadlener
Copy link
Contributor Author

Thanks a lot :)

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