Page MenuHomePhabricator

[Sema] Add casting check for fixed to fixed point conversions
ClosedPublic

Authored by vabridgers on Aug 3 2020, 1:40 PM.

Details

Summary

This change squelches the warning for a cast from fixed to fixed point
conversions when -Wbad-function-cast is enabled.

Fixes:

cast from function call of type '_Fract' to non-matching type '_Fract'
[-Wbad-function-cast]

Diff Detail

Event Timeline

vabridgers created this revision.Aug 3 2020, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2020, 1:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vabridgers requested review of this revision.Aug 3 2020, 1:40 PM
bjope added a subscriber: bjope.Aug 3 2020, 3:39 PM
bjope added inline comments.
clang/lib/Sema/SemaCast.cpp
2660

Is this really the intention with the patch?

It does match the "summary" above, but isn't the warning well deserved for int->fixed cast.

I also base that question on the you referred to this patch from our bugtracker at Ericsson, which was about conversions from long __fixed to long __fixed. So I kind of expected to find (SrcType->isFixedPointType() && DestType->isFixedPointType()) here (similar to the checks above that avoids warning when SrcType equals the DestType).

The test case I expected (related to our downstream bug report) would look like this:

_Fract ff1(void);

void
foo(void)
{
  (_Fract)ff1();  // No warning expected.
}
clang/test/Sema/warn-bad-function-cast.c
2

I doubt it is worth running the whole test twice just to test both with and without -ffixed-point, specially since there are no differences in the earlier tests when enabling -ffixed-point. So just adding -ffixed-point to the pre-existing RUN-line would save us from running this test case a humongous number of extra times during the next decade.

54

This should be added before the line saying /* All following casts issue warning */.

54

Is the (void) needed/relevant here?

bjope added inline comments.Aug 3 2020, 3:49 PM
clang/test/Sema/warn-bad-function-cast.c
54

As questioned earlier, shouldn't we expect a warning for this scenario?

There is however a problem that we get the warning for _Fract to _Fract conversion. And it would be nice with a more complete set of tests involving both FixedPoint->FixedPoint, FixedPoint->Integer and Integer->FixedPoint casts.

vabridgers updated this revision to Diff 282775.Aug 3 2020, 5:33 PM

improve the commit message detail

vabridgers marked 5 inline comments as done.Aug 3 2020, 5:36 PM

I updated the commit header with more details since the first submission was obviously too terse. @bjope, I believe the update should address your comments.

clang/lib/Sema/SemaCast.cpp
2660

I improve the commit message detail. Please read and let me know if the intent is still not clear.

clang/test/Sema/warn-bad-function-cast.c
2

Well, I had a few choices, seems I picked the red pill compared to what you want :) Look at the change, see if you're happy with that. Of course, the owner will need to agree.

vabridgers updated this revision to Diff 282776.Aug 3 2020, 5:40 PM
vabridgers marked 2 inline comments as done.

remove -DFIXED_POINT from lit test, since it's not needed in this casting

clang/test/Sema/warn-bad-function-cast.c
54

If you have any *specific* suggestions for test cases, I'm open to that.

bjope added inline comments.Aug 3 2020, 11:41 PM
clang/lib/Sema/SemaCast.cpp
2660

I'm afraid you didn't update the summary in phabricator? (I've never managed to do that using arcanist myself, after having updated the commit msg locally I usually need to update the description in phabricator using the web ui).

2660

Is this really the intention with the patch?

It does match the "summary" above, but isn't the warning well deserved for int->fixed cast.

I also base that question on the you referred to this patch from our bugtracker at Ericsson, which was about conversions from long __fixed to long __fixed. So I kind of expected to find (SrcType->isFixedPointType() && DestType->isFixedPointType()) here (similar to the checks above that avoids warning when SrcType equals the DestType).

The test case I expected (related to our downstream bug report) would look like this:

_Fract ff1(void);

void
foo(void)
{
  (_Fract)ff1();  // No warning expected.
}
2660

Is this really the intention with the patch?

It does match the "summary" above, but isn't the warning well deserved for int->fixed cast.

I also base that question on the you referred to this patch from our bugtracker at Ericsson, which was about conversions from long __fixed to long __fixed. So I kind of expected to find (SrcType->isFixedPointType() && DestType->isFixedPointType()) here (similar to the checks above that avoids warning when SrcType equals the DestType).

The test case I expected (related to our downstream bug report) would look like this:

_Fract ff1(void);

void
foo(void)
{
  (_Fract)ff1();  // No warning expected.
}
clang/test/Sema/warn-bad-function-cast.c
54

Add:

_Fract ff1(void);

And inside foo add these three tests (you'll need to add appropriate expects):

(_Fract)ff1();  // No warning expected.
(_Fract)if1();  // Warning expected.
(int)ff1();  // Warning expected.
vabridgers edited the summary of this revision. (Show Details)Aug 4 2020, 5:27 AM
vabridgers edited the summary of this revision. (Show Details)
vabridgers edited the summary of this revision. (Show Details)Aug 4 2020, 5:31 AM
vabridgers retitled this revision from [Sema] Add casting check for integer to fixed point conversions to [Sema] Add casting check for fixed to fixed point conversions.Aug 4 2020, 6:40 AM
vabridgers edited the summary of this revision. (Show Details)
vabridgers edited the summary of this revision. (Show Details)
vabridgers updated this revision to Diff 282902.Aug 4 2020, 6:42 AM
vabridgers edited the summary of this revision. (Show Details)

ok, I think it's all sorted out now.

ok, I think it's all sorted out now. Thanks @bevinh for the desk review. Let's start at the beginning again :)

Ping! Ok to land?

bjope added inline comments.Aug 6 2020, 11:39 PM
clang/test/Sema/warn-bad-function-cast.c
54

I still think it would be nice not to break the structure of this test. Tests seem to be divided into three categories:

/* Casts to void types are always OK. */

/* Casts to the same type or similar types are OK.  */

/* All following casts issue warning */

And you have currently inserted all new tests in the last section.

vabridgers updated this revision to Diff 283858.Aug 7 2020, 3:07 AM

address Bjorn's comment

vabridgers marked an inline comment as done.Aug 7 2020, 3:08 AM
vabridgers added inline comments.
clang/test/Sema/warn-bad-function-cast.c
54

When I've seen bew

54

Done. ok to land?

vabridgers marked an inline comment as done.Aug 7 2020, 3:09 AM
vabridgers marked 4 inline comments as done.

All comments marked as done. ok to land?

bjope accepted this revision.Aug 7 2020, 4:37 AM

LGTM!

This revision is now accepted and ready to land.Aug 7 2020, 4:37 AM