Skip to content

Conversation

@brgr
Copy link
Collaborator

@brgr brgr commented Jun 9, 2022

  • warn at equality checks (generally always besides when we can guarantee that it holds)
  • warn at operations where a number possibly gets completely consumed (e.g. DBL_MAX - 1.0, which will just result in DBL_MAX again)
    • as for this, we could actually warn always when we don't expect an exact result - But is that not probably too often the case? We need to think about this...

First is CWE 1077
For the second point I have found these that fit:

@brgr brgr force-pushed the add-further-warnings branch from 816dfba to 784b8e8 Compare June 9, 2022 14:19
@brgr brgr requested review from Dudeldu and FelixKrayer June 9, 2022 14:22
Copy link
Owner

@Dudeldu Dudeldu left a comment

Choose a reason for hiding this comment

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

I really like the warning showing exactly where you lost the "exactness".

int check2 = (x == y); // WARN

// The following is __DBL_MAX__
double my_max = 179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000;
Copy link
Owner

Choose a reason for hiding this comment

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

Add the header float.h, so you can use DBL_MAX instead of this interesting number

Copy link
Collaborator

@FelixKrayer FelixKrayer Jun 12, 2022

Choose a reason for hiding this comment

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

I guess this is the same issue i had with DBL_MIN ? #13 (comment)

Copy link
Owner

Choose a reason for hiding this comment

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

Right, I also added the DBL_MAX case to my fix

Copy link
Owner

Choose a reason for hiding this comment

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

With #10 merged, we should now be able to use DBL_MAX constant.

@brgr brgr force-pushed the add-further-warnings branch from acf0cd3 to 2318f94 Compare June 14, 2022 17:14
FelixKrayer
FelixKrayer previously approved these changes Jun 15, 2022
@Dudeldu
Copy link
Owner

Dudeldu commented Jun 15, 2022

Why are the regression tests still failing? Seems like there is no warning generated for the first comparison.

@Dudeldu Dudeldu merged commit 7d5b149 into master Jun 16, 2022
brgr added a commit that referenced this pull request Jun 16, 2022
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.

4 participants