Page MenuHomePhabricator

[SEMA] add more -Wfloat-conversion to compound assigment analysis

Authored by nickdesaulniers on Aug 8 2018, 10:56 AM.

Diff Detail


Event Timeline

  • rework ordering of conditionals to reduce indentation
10413–10416 ↗(On Diff #159763)

This is essentially the only new code. AnalyzeCompoundAssignment was moved in order to be able to call DiagnoseFloatingImpCast (it's defined above). The rest is a small refactoring of the conditionals.

pirama added inline comments.Aug 8 2018, 3:30 PM
10411 ↗(On Diff #159763)

Add a comment explaining this conditional as well?

Return if source and target types are unavailable or if source is not a floating point.

With the comment, it might be cleaner to read if you expand the negation: !ResultBT || !RBT || !RBT->isFloatingPoint()

nickdesaulniers added inline comments.Aug 8 2018, 3:47 PM
10411 ↗(On Diff #159763)

ResultBT and RBT are pointers that may be null and need to be checked. The previous code did this before accessing them, I've just moved the checks to be sooner.

If !RBT (if it's nullptr), then !RBT->isFloatingPoint() would be a null deref, so unfortunately I can not expand it as recommended.

nickdesaulniers added inline comments.Aug 8 2018, 3:51 PM
10411 ↗(On Diff #159763)

oops, nevermind

  • clean up conditional and add comment
nickdesaulniers marked an inline comment as done.Aug 8 2018, 3:54 PM
acoomans accepted this revision.Aug 10 2018, 11:46 AM
acoomans added a subscriber: acoomans.


This revision is now accepted and ready to land.Aug 10 2018, 11:46 AM
aaron.ballman requested changes to this revision.Aug 10 2018, 11:53 AM
aaron.ballman added inline comments.
10424 ↗(On Diff #159820)

This looks incorrect to me -- this is testing the rank difference and that it is in a system macro (the ! was dropped). If this didn't cause any tests to fail, we should add a test that would fail for it.

This revision now requires changes to proceed.Aug 10 2018, 11:53 AM
10424 ↗(On Diff #159820)

Thanks for the code review!

Sorry, I don't understand what you mean by "rank difference"? F16 vs F32 vs F64 vs F128 etc?

When you say "this looks incorrect," are you commenting on the code I added (Lines 10415 to 10418) or the prexisting code that I moved (Lines 10420-10427)?

Good catch on dropping the !, that was definitely not intentional (and now I'm curious if dw in vim will delete (! together), will add back.

I'm happy to add a test for the system macro, but I also don't know what that is. Can you explain?

aaron.ballman added inline comments.Aug 10 2018, 1:18 PM
10424 ↗(On Diff #159820)

Sorry, my explanation may have been confusing. A better way to phrase it would have been "this isn't doing what the old code used to do, and I don't think you intended to change it." I was talking about the ! that disappeared.

I think you can use linemarker directives to get this to happen:

# 1 "systemheader.h" 1 3
#define MACRO

# 1 "test_file.c" 2
// Your code that uses MACRO.
  • fix up system macro case and add test coverage for that case.
nickdesaulniers marked 3 inline comments as done.Aug 10 2018, 3:18 PM

Thanks for the info, I found helpful. How does this look?

This revision is now accepted and ready to land.Aug 12 2018, 7:40 AM
This revision was automatically updated to reflect the committed changes.

Thank you for the code review.