Bug 48956 - -Wconversion should warn when a complex value is assigned to a real result
Summary: -Wconversion should warn when a complex value is assigned to a real result
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 4.6.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, easyhack
: 50992 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-05-10 23:55 UTC by stevenj
Modified: 2015-06-25 09:16 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-05-11 08:13:47


Attachments
patch to add a -Wconversion warning for complex -> real conversions (567 bytes, patch)
2011-05-11 20:16 UTC, stevenj
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description stevenj 2011-05-10 23:55:42 UTC
Consider the following C snippet:

  #include <complex.h>
  double foo(complex double x) {
     return x;
  }

This is valid C code: it assigns the complex "x" value to the real result of "foo", which implicitly returns the real part of "x".  However, the implicit conversion from complex to real has altered a value by discarding the imaginary part of x.  Therefore, I would expect -Wconversion to issue a warning.

Compiling this with "gcc -c -Wconversion foo.c" issues no warning, however, nor can I find any other -W option that causes a warning to be emitted.

Please consider issuing a warning when implicitly converting complex to real (NOT the reverse) on -Wconversion.  (In fact, -Wconversion already does this in Fortran as noted below.)

A warning for this type of conversion would be very helpful, since silently discarding the imaginary part in this way often indicates a bug in my experience.  (One can come up with many similar examples, e.g. writing sqrt(x) rather than csqrt(x) currently silently succeeds for complex x, again discarding the imaginary part.)  If the programmer meant to take the real part, she probably would have written "creal(x)" explicitly.

Regards,
Steven G. Johnson

PS. Note -Wconversion already DOES emit a warning for conversion to complex to real in gfortran.  e.g.

      subroutine foo(z)
      complex z
      real x
      x = z
      write(*,*) x
      return
      end

compiles without error with gfortran -c, but gfortran -c -Wconversion emits:

      x = z                                                             
         1
Warning: Conversion from COMPLEX(4) to REAL(4) at (1)
Comment 1 stevenj 2011-05-11 00:01:32 UTC
PPS. I have reproduced this problem in gcc versions 3.3.6, 3.4.6, 4.1.3, 4.3.2, and 4.6.0 running on an x86_64 Debian GNU/Linux system.
Comment 2 Manuel López-Ibáñez 2011-05-11 08:13:47 UTC
The Wconversion code makes not attempt to identify conversion from/to complex value but it is probably easy to implement. 

If you are really interested in this, I can give you some hints and advice and you will probably get it ready for GCC 4.7, but I don't have free time to do it myself, sorry.
Comment 3 Manuel López-Ibáñez 2011-05-11 08:16:18 UTC
(In reply to comment #0)
> PS. Note -Wconversion already DOES emit a warning for conversion to complex to
> real in gfortran.  e.g.

Except for very few exceptions, the diagnostic machinery of Fortran is independent of the diagnostic machinery of C/C++ (in some cases worse, in some cases better). On the other hand, C/C++ share most of the code for Wconversion, so implementing it for C will make it available for C++.
Comment 4 stevenj 2011-05-11 20:16:57 UTC
Created attachment 24230 [details]
patch to add a -Wconversion warning for complex -> real conversions

I believe the attached patch (against gcc 4.6.0) implements the feature I requested.  It emits a warning for implicit conversions of complex types to real (or integer) types.  It also emits a warning for similarly converting complex constants, unless the imaginary part of the constant is zero in which case it is treated as a real (or integer) constant given by the real part.

Note that complex->complex conversions are still not checked (e.g. no warning is given in converting a complex double to a complex float, etcetera), but at least I didn't add any regressions in that regard.

For example, when compiling the following test file:

#include <complex.h>
#include <math.h>

double foo(double complex z)
{
     complex double zc = 1 + 1i;
     complex double zc0 = 1 + 0i;

     float complex zf = z;

     double x = z;
     float xf = z;
     int i = z;

     double xc = 1 + 1i;
     double xc0 = 1 + 0i;
     int ic = 1.1 + 1.1i;
     int ic0 = 1.1 + 0.0i;

     return sqrt(z) + csqrt(z);
}

with "gcc -Wconversion -c", the unpatched gcc gives no warnings whereas the patched gcc emits:

complex-Wconversion-tst.c: In function ‘foo’:
complex-Wconversion-tst.c:11:6: warning: conversion to ‘double’ from ‘complex double’ may alter its value [-Wconversion]
complex-Wconversion-tst.c:12:6: warning: conversion to ‘float’ from ‘complex double’ may alter its value [-Wconversion]
complex-Wconversion-tst.c:13:6: warning: conversion to ‘int’ from ‘complex double’ may alter its value [-Wconversion]
complex-Wconversion-tst.c:15:6: warning: conversion to ‘double’ alters ‘complex int’ constant value [-Wconversion]
complex-Wconversion-tst.c:17:6: warning: conversion to ‘int’ alters ‘complex double’ constant value [-Wconversion]
complex-Wconversion-tst.c:18:6: warning: conversion to ‘int’ alters ‘double’ constant value [-Wconversion]
complex-Wconversion-tst.c:20:6: warning: conversion to ‘double’ from ‘complex double’ may alter its value [-Wconversion]
complex-Wconversion-tst.c:20:21: warning: conversion to ‘double’ from ‘complex double’ may alter its value [-Wconversion]

Note that two warnings are emitted for the last line: one for the implicit conversion of z to a real number in sqrt(z), and the other for the implicit conversion of the complex sqrt(z)+csqrt(z) to a real return value of foo(z).

Using creal(...) and/or explicit casts silences the warnings.  Note also that "double xc0 = 1 + 0i;" correctly yields no warning.

As mentioned above, "float complex zf = z;" and similar complex->complex conversions probably should emit a warning, but I have not implemented that.  However, that is not a regression.

--SGJ
Comment 5 Manuel López-Ibáñez 2011-05-11 21:57:54 UTC
(In reply to comment #4)
> Created attachment 24230 [details]
> patch to add a -Wconversion warning for complex -> real conversions
> 
> I believe the attached patch (against gcc 4.6.0) implements the feature I
> requested.  It emits a warning for implicit conversions of complex types to
> real (or integer) types.  It also emits a warning for similarly converting
> complex constants, unless the imaginary part of the constant is zero in which
> case it is treated as a real (or integer) constant given by the real part.

Nice! The patch looks almost ok but I cannot approve it. 

Some small issues:

* The code must follow GCC style, which is described in detail here http://gcc.gnu.org/contribute.html#standards, but it is just easier too look code around and follow the same style for braces, comments (they have to start uppercase and end with a period), you have to add /* Fall through. */ to mark deliberate fall-through in switches, etc.

* I would say (TREE_CODE (type) != COMPLEX_TYPE) rather than (!(TREE_CODE (type) == COMPLEX_TYPE)). Even better, test first that == COMPLEX_TYPE, if so you can return, otherwise, the zero imaginary part, else warning. Does that sound right?

* You need to add the testcase to the testsuite. Look in testsuite/gcc.dg/c-c++-common/Wconversion-real.c for an example. More info: http://gcc.gnu.org/wiki/HowToPrepareATestcase

* Patches should be always be done against a recent SVN revision. I think this patch should be safe to backport to GCC 4.6, but perhaps the release managers may disagree (it does generate new warnings, which some people may find bothersome if they use -Werror).

* Patches need to be bootstrapped and regression tested: http://gcc.gnu.org/contribute.html#testing . There are scripts that mostly automate this process. Take a look at contrib/ or this script of mine: http://gcc.gnu.org/wiki/ManuelL%C3%B3pezIb%C3%A1%C3%B1ez?action=AttachFile&do=view&target=gccfarming

* Finally, patches should be sent to gcc-patches@gcc.gnu.org with a Changelog. Take a look at the archives to have an idea of the proper way: http://gcc.gnu.org/ml/gcc-patches/2011-05/
Comment 6 stevenj 2011-05-12 22:33:59 UTC
Thanks, I was somewhat aware of the additional requirements for applying patches to the official tree (probably I should also file a copyright assignment), but I wanted to check with you that I was on the right track first.   I'm a bit swamped this week, but should be able to do all this next week.
Comment 7 Manuel López-Ibáñez 2011-05-12 23:06:14 UTC
(In reply to comment #6)
> Thanks, I was somewhat aware of the additional requirements for applying
> patches to the official tree (probably I should also file a copyright
> assignment), but I wanted to check with you that I was on the right track
> first.   I'm a bit swamped this week, but should be able to do all this next
> week.

You should definitely get the copyright assignment, but you do not need to wait for it to submit the patch properly and get more feedback and approval.
Comment 8 Manuel López-Ibáñez 2012-03-24 19:14:49 UTC
*** Bug 50992 has been marked as a duplicate of this bug. ***
Comment 9 Paolo Carlini 2014-11-07 10:13:54 UTC
Out of curiosity, what happened to this issue? (is still assigned to Steven) Note that as far as I can see it's essentially a C issue, because the C++ front-end rejects this kind of code with an hard error.
Comment 10 Manuel López-Ibáñez 2014-11-07 12:57:04 UTC
(In reply to Paolo Carlini from comment #9)
> Out of curiosity, what happened to this issue? (is still assigned to Steven)
> Note that as far as I can see it's essentially a C issue, because the C++
> front-end rejects this kind of code with an hard error.

I think it is safe to assume that anyone is welcome to follow up on this. See also comment #5. I personally would leave this as one of the EasyHacks for new contributors. It is easy to fix but not very important.
Comment 11 Mikhail Maltsev 2015-01-01 18:15:42 UTC
Proposing a new patch: https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01925.html
Comment 12 Mikhail Maltsev 2015-05-15 18:03:21 UTC
Author: miyuki
Date: Fri May 15 18:02:50 2015
New Revision: 223223

URL: https://gcc.gnu.org/viewcvs?rev=223223&root=gcc&view=rev
Log:
PR c/48956

gcc/c-family/
* c-common.c (int_safely_convertible_to_real_p): Define.
(unsafe_conversion_p): Check conversions involving complex types.
(conversion_warning): Add new warning message for conversions which
discard imaginary component.
* c-common.h: (enum conversion_safety): Add new enumerator for such
conversions.

gcc/testsuite/
* gcc.dg/Wconversion-complex-c99.c: New test.
* gcc.dg/Wconversion-complex-gnu.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/Wconversion-complex-c99.c
    trunk/gcc/testsuite/gcc.dg/Wconversion-complex-gnu.c
Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/c-family/c-common.h
    trunk/gcc/testsuite/ChangeLog
Comment 13 Manuel López-Ibáñez 2015-05-15 18:47:49 UTC
(In reply to Mikhail Maltsev from comment #12)

Great! Please do not forget to close the bug (using your @gcc.gnu.org account). I leave the honor to you. Perhaps even worth it to add this to https://gcc.gnu.org/gcc-6/changes.html ;)
Comment 14 Mikhail Maltsev 2015-05-15 18:55:52 UTC
> I leave the honor to you.
Thanks ;).
Fixed for GCC 6.
Comment 15 Matt Kline 2015-06-24 21:53:22 UTC
> Fixed for GCC 6.

May I ask why this is being deferred until GCC 6.x? I'll readily admit that I'm not well-versed in the GCC release cycle, but this seems like a trivial enhancement and would be useful immediately. My team has been bitten by these silent conversions, and -Wconversion on Clang currently provides a similar warning.
Comment 16 Manuel López-Ibáñez 2015-06-24 22:08:35 UTC
(In reply to Matt Kline from comment #15)
> > Fixed for GCC 6.
> 
> May I ask why this is being deferred until GCC 6.x? I'll readily admit that
> I'm not well-versed in the GCC release cycle, but this seems like a trivial
> enhancement and would be useful immediately. My team has been bitten by
> these silent conversions, and -Wconversion on Clang currently provides a
> similar warning.

GCC 6 is the next development release, which will be released early next year. GCC 5 is the current stable release and, normally, only bugfixes are added to it. If you wish to propose a backport, you'll need to write to the release managers (for example, by writing to gcc@) and ask for permission, then you need to do (or find someone to do) the backport, test it and commit it.

I'd rather have two new features in GCC 6 than one new feature in both GCC 5 and GCC 6, thus I (and likely others) prefer to dedicate my time to fixing other things (and there is a lot of work to do: https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps) rather than on back-porting new features.
Comment 17 Matt Kline 2015-06-24 22:13:27 UTC
Thanks for the info and such a quick response! I'll see if I can do the required legwork.
Comment 18 Marek Polacek 2015-06-25 09:16:43 UTC
Backporting a new warning to a released branch isn't viable I'm afraid, it would make upgrading from 5.1 to 5.2 unsafe for some users.