Skip to content

Conversation

biojppm
Copy link
Contributor

@biojppm biojppm commented Nov 15, 2020

re #34

@lemire
Copy link
Member

lemire commented Nov 15, 2020

Thanks. We deliberately want C++ to treat the objects as trivial, hence the default constructor.

I have added inline initializer which should hopefully do away with the warning. Please sync your branch.

@biojppm
Copy link
Contributor Author

biojppm commented Nov 15, 2020

@lemire The previous changes do reproduce the problem in the CI.

I think it is a genuine one: in absence of ctor initialization, this statement will return mantissa uninitialized:

image

@biojppm
Copy link
Contributor Author

biojppm commented Nov 15, 2020

I see you've added a fix. Let me try it.

@biojppm
Copy link
Contributor Author

biojppm commented Nov 15, 2020

It's curious that later gcc versions fail to point to this issue.

@biojppm biojppm changed the title fix g++-6 maybe-uninitialized [WIP] fix g++-6 maybe-uninitialized Nov 15, 2020
@lemire
Copy link
Member

lemire commented Nov 15, 2020

It's curious that later gcc versions fail to point to this issue.

I submit to you that it is because it is not an issue.

I think it is a genuine one: in absence of ctor initialization, this statement will return mantissa uninitialized:

I disagree. Here is the code...

  adjusted_mantissa am = pns.too_many_digits ? parse_long_mantissa<binary_format<T>>(first,last) : compute_float<binary_format<T>>(pns.exponent, pns.mantissa);
  if(am.power2 < 0) { am = parse_long_mantissa<binary_format<T>>(first,last); }

See how if am.power2 is negative, then mantissa is never used?

It is not needed that mantissa ever be initialized if it is never being read.

If the compiler is not smart enough, its analysis will end up with a false positive.

@biojppm biojppm changed the title fix g++-6 maybe-uninitialized re #34: reproduce in CI Nov 15, 2020
@lemire
Copy link
Member

lemire commented Nov 15, 2020

Great PR. Merging.

@lemire lemire merged commit db539b2 into fastfloat:main Nov 15, 2020
@biojppm
Copy link
Contributor Author

biojppm commented Nov 15, 2020

Thanks for clarifying.

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