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

[vabridgers]

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]

[bjope]

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.

49

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

49

Is the (void) needed/relevant here?

[bjope]

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

49

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]

improve the commit message detail

[vabridgers]

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]

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

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

49

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

[bjope]

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

49

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]

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

[vabridgers]

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

[vabridgers]

Ping! Ok to land?

[bjope]

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

49

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]

address Bjorn's comment

[vabridgers]

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

49

When I've seen bew

49

Done. ok to land?

[vabridgers]

All comments marked as done. ok to land?

[bjope]

LGTM!

clang/lib/Sema/SemaCast.cpp

static void DiagnoseBadFunctionCast(Sema &Self, const ExprResult &SrcExpr,
   if (SrcType->isRealFloatingType() && DestType->isRealFloatingType())
     return;
   if (SrcType->isEnumeralType() && DestType->isEnumeralType())
     return;
   if (SrcType->isComplexType() && DestType->isComplexType())
     return;
   if (SrcType->isComplexIntegerType() && DestType->isComplexIntegerType())
     return;
+  if (SrcType->isIntegerType() && DestType->isFixedPointType())
+    return;
 
   Self.Diag(SrcExpr.get()->getExprLoc(),
             diag::warn_bad_function_cast)
             << SrcType << DestType << SrcExpr.get()->getSourceRange();
 }
 
 /// Check the semantics of a C-style cast operation, in C.
 void CastOperation::CheckCStyleCast() {

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

 // RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wbad-function-cast -triple x86_64-unknown-unknown -verify
+// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wbad-function-cast -DFIXED_POINT -ffixed-point -triple x86_64-unknown-unknown -verify
 // rdar://9103192
 
 void vf(void);
 int if1(void);
 char if2(void);
 long if3(void);
 float rf1(void);
 double rf2(void);
foo(void)
   (_Bool)if3(); /* expected-warning {{cast from function call of type 'long' to non-matching type '_Bool'}} */
   (int)rf1(); /* expected-warning {{cast from function call of type 'float' to non-matching type 'int'}} */
   (long)rf2(); /* expected-warning {{cast from function call of type 'double' to non-matching type 'long'}} */
   (double)cf(); /* expected-warning {{cast from function call of type '_Complex double' to non-matching type 'double'}} */
   (int)ef(); /* expected-warning {{cast from function call of type 'enum e' to non-matching type 'int'}} */
   (int)bf(); /* expected-warning {{cast from function call of type '_Bool' to non-matching type 'int'}} */
   (__SIZE_TYPE__)pf1(); /* expected-warning {{cast from function call of type 'char *' to non-matching type 'unsigned long'}} */
   (__PTRDIFF_TYPE__)pf2(); /* expected-warning {{cast from function call of type 'int *' to non-matching type 'long'}} */
+
+#ifdef FIXED_POINT
+  (void)(_Fract) if1(); // no warning
+#endif
 }