[Home]

Summary:ASTERISK-16065: [patch] Eliminate compiler warning in app_voicemail.c
Reporter:John Covert (jcovert)Labels:
Date Opened:2010-05-06 14:27:32Date Closed:2010-05-11 12:25:28
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/app_voicemail
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) app_voicemail.c.patch
Description:There's a harmless but incorrect bit of code in app_voicemail which produces the following compiler warning with the default compiler options on Darwin:

app_voicemail.c: In function 'vm_execmain':
app_voicemail.c:9346: warning: control may reach end of non-void function 'vm_intro' being inlined

It is strongly recommended C coding practice to ensure that the final statement in any non-void function is a return with a value.  While the code here as currently written would always execute a return with a value, it would be better quality and less likely to grow ugly scales due to a future change if it were corrected as indicated in the attached patch.

The patch is relative to trunk.
Comments:By: Tilghman Lesher (tilghman) 2010-05-06 15:04:38

What compiler version is giving you that warning?
By: John Covert (jcovert) 2010-05-06 15:09:37

# gcc -v
Using built-in specs.
Target: powerpc-apple-darwin8
Configured with: /var/tmp/gcc/gcc-5370~2/src/configure --disable-checking -enable-werror --prefix=/usr --mandir=/share/man --enable-languages=c,objc,c++,obj-c++ --program-transform-name=/^[cg][^.-]*$/s/$/-4.0/ --with-gxx-include-dir=/include/c++/4.0.0 --with-slibdir=/usr/lib --build=powerpc-apple-darwin8 --host=powerpc-apple-darwin8 --target=powerpc-apple-darwin8
Thread model: posix
gcc version 4.0.1 (Apple Computer, Inc. build 5370)
By: Jason Parker (jparker) 2010-05-06 15:22:10

This is a regression in gcc that has been fixed.  I suspect that "if (0) {}" above it is throwing the optimizer for a loop.  There are other functions (such as vm_instructions()) that have similar return logic, and work fine.
By: Jason Parker (jparker) 2010-05-06 15:23:29

For reference: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=21191  (note the statically defined branch, rather than the inlining)
By: John Covert (jcovert) 2010-05-06 16:46:43

If you read the comments at http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19699 it's not clear that the statement in 21191 that this is "not bad style" is valid.

Note the issue of "false positives" vs "false negatives".

The code would be much more robust, especially since the statements above my proposed fix are subject to constant revision, if you apply my patch.
By: Tilghman Lesher (tilghman) 2010-05-06 17:59:45

The patch in ASTERISK-15519 fixes this in a different way, and that's likely where we're going to go.  When there's a bug in the compiler, we'll switch off the compiler optimization which triggers that bug.

We do NOT code around compiler bugs (or bugs in any other code).
By: John Covert (jcovert) 2010-05-06 18:22:46

I'm not suggesting you "code around a compiler bug", because it hasn't been agreed that there is a compiler bug.  But I am suggesting that the existing long sequence of "if then else if then else if then else" with the default condition in the final "else" would be (a) more robust and (b) more maintainable if the default condition simply were removed from the if then else chain and made an independent final statement.

It's bad style for the final statement in a non-void function to be conditional.

I think you referenced the wrong patch in the previous reply.
By: Tilghman Lesher (tilghman) 2010-05-06 18:56:36

1) In fact, it was agreed that it was a compiler bug, and it was fixed.  The remark about "false positives" was a question about whether it would be fixed in gcc 4.0; since it was a diagnostic error, only, and because the change was invasive, the maintainers elected against it.
2) The code is completely maintainable, as is, and it was intentionally coded that way to make it more maintainable.
3) Please cite the relevant style guide where a conditional cannot be the final construct in a non-void function.
4) You are incorrect.  I referenced the correct patch in the previous reply.
By: John Covert (jcovert) 2010-05-06 19:23:02

For example: http://tinyurl.com/36f86aw

16.8: "All exit paths from a function with non-void return type shall have an explicit return statement with an expression." "Missing return value for non-void function XX." "Warning when a non-void function is not terminated with an unconditional return with an expression."
By: Tilghman Lesher (tilghman) 2010-05-06 19:58:12

That style guide does not prohibit the construct as we have built it.  All returns are explicit; there are no code paths where an implicit return can occur.
By: Digium Subversion (svnbot) 2010-05-10 11:34:22

Repository: asterisk
Revision: 262151

U   branches/1.4/Makefile.rules

------------------------------------------------------------------------
r262151 | tilghman | 2010-05-10 11:34:21 -0500 (Mon, 10 May 2010) | 10 lines

Allow compilation on Mac OS X 10.4 (Tiger)

(closes issue ASTERISK-15519)
Reported by: jcovert
Patches:
      20100506__issue17297.diff.txt uploaded by tilghman (license 14)

(closes issue ASTERISK-16065)
Reported by: jcovert

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262151
By: Digium Subversion (svnbot) 2010-05-10 11:36:26

Repository: asterisk
Revision: 262152

_U  trunk/
U   trunk/Makefile.rules

------------------------------------------------------------------------
r262152 | tilghman | 2010-05-10 11:36:25 -0500 (Mon, 10 May 2010) | 17 lines

Merged revisions 262151 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r262151 | tilghman | 2010-05-10 11:34:21 -0500 (Mon, 10 May 2010) | 10 lines
 
 Allow compilation on Mac OS X 10.4 (Tiger)
 
 (closes issue ASTERISK-15519)
  Reported by: jcovert
  Patches:
        20100506__issue17297.diff.txt uploaded by tilghman (license 14)
 
 (closes issue ASTERISK-16065)
  Reported by: jcovert
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262152
By: Digium Subversion (svnbot) 2010-05-10 11:38:47

Repository: asterisk
Revision: 262153

_U  branches/1.6.0/
U   branches/1.6.0/Makefile.rules

------------------------------------------------------------------------
r262153 | tilghman | 2010-05-10 11:38:46 -0500 (Mon, 10 May 2010) | 24 lines

Merged revisions 262152 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r262152 | tilghman | 2010-05-10 11:36:25 -0500 (Mon, 10 May 2010) | 17 lines
 
 Merged revisions 262151 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r262151 | tilghman | 2010-05-10 11:34:21 -0500 (Mon, 10 May 2010) | 10 lines
   
   Allow compilation on Mac OS X 10.4 (Tiger)
   
   (closes issue ASTERISK-15519)
    Reported by: jcovert
    Patches:
          20100506__issue17297.diff.txt uploaded by tilghman (license 14)
   
   (closes issue ASTERISK-16065)
    Reported by: jcovert
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262153
By: Digium Subversion (svnbot) 2010-05-10 11:38:55

Repository: asterisk
Revision: 262154

_U  branches/1.6.1/
U   branches/1.6.1/Makefile.rules

------------------------------------------------------------------------
r262154 | tilghman | 2010-05-10 11:38:54 -0500 (Mon, 10 May 2010) | 24 lines

Merged revisions 262152 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r262152 | tilghman | 2010-05-10 11:36:25 -0500 (Mon, 10 May 2010) | 17 lines
 
 Merged revisions 262151 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r262151 | tilghman | 2010-05-10 11:34:21 -0500 (Mon, 10 May 2010) | 10 lines
   
   Allow compilation on Mac OS X 10.4 (Tiger)
   
   (closes issue ASTERISK-15519)
    Reported by: jcovert
    Patches:
          20100506__issue17297.diff.txt uploaded by tilghman (license 14)
   
   (closes issue ASTERISK-16065)
    Reported by: jcovert
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262154
By: Digium Subversion (svnbot) 2010-05-10 11:39:04

Repository: asterisk
Revision: 262155

_U  branches/1.6.2/
U   branches/1.6.2/Makefile.rules

------------------------------------------------------------------------
r262155 | tilghman | 2010-05-10 11:39:03 -0500 (Mon, 10 May 2010) | 24 lines

Merged revisions 262152 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r262152 | tilghman | 2010-05-10 11:36:25 -0500 (Mon, 10 May 2010) | 17 lines
 
 Merged revisions 262151 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r262151 | tilghman | 2010-05-10 11:34:21 -0500 (Mon, 10 May 2010) | 10 lines
   
   Allow compilation on Mac OS X 10.4 (Tiger)
   
   (closes issue ASTERISK-15519)
    Reported by: jcovert
    Patches:
          20100506__issue17297.diff.txt uploaded by tilghman (license 14)
   
   (closes issue ASTERISK-16065)
    Reported by: jcovert
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262155
By: Digium Subversion (svnbot) 2010-05-11 12:22:09

Repository: asterisk
Revision: 262321

U   branches/1.4/Makefile.rules
U   branches/1.4/apps/app_voicemail.c

------------------------------------------------------------------------
r262321 | tilghman | 2010-05-11 12:22:08 -0500 (Tue, 11 May 2010) | 2 lines

Fix issue ASTERISK-16065 a slightly different way (mad props to Qwell)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262321
By: Digium Subversion (svnbot) 2010-05-11 12:23:51

Repository: asterisk
Revision: 262330

_U  trunk/
U   trunk/Makefile.rules
U   trunk/apps/app_voicemail.c

------------------------------------------------------------------------
r262330 | tilghman | 2010-05-11 12:23:51 -0500 (Tue, 11 May 2010) | 9 lines

Merged revisions 262321 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r262321 | tilghman | 2010-05-11 12:22:07 -0500 (Tue, 11 May 2010) | 2 lines
 
 Fix issue ASTERISK-16065 a slightly different way (mad props to Qwell)
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262330
By: Digium Subversion (svnbot) 2010-05-11 12:25:12

Repository: asterisk
Revision: 262337

_U  branches/1.6.0/
U   branches/1.6.0/Makefile.rules
U   branches/1.6.0/apps/app_voicemail.c

------------------------------------------------------------------------
r262337 | tilghman | 2010-05-11 12:25:11 -0500 (Tue, 11 May 2010) | 16 lines

Merged revisions 262330 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r262330 | tilghman | 2010-05-11 12:23:51 -0500 (Tue, 11 May 2010) | 9 lines
 
 Merged revisions 262321 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r262321 | tilghman | 2010-05-11 12:22:07 -0500 (Tue, 11 May 2010) | 2 lines
   
   Fix issue ASTERISK-16065 a slightly different way (mad props to Qwell)
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262337
By: Digium Subversion (svnbot) 2010-05-11 12:25:21

Repository: asterisk
Revision: 262339

_U  branches/1.6.1/
U   branches/1.6.1/Makefile.rules
U   branches/1.6.1/apps/app_voicemail.c

------------------------------------------------------------------------
r262339 | tilghman | 2010-05-11 12:25:20 -0500 (Tue, 11 May 2010) | 16 lines

Merged revisions 262330 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r262330 | tilghman | 2010-05-11 12:23:51 -0500 (Tue, 11 May 2010) | 9 lines
 
 Merged revisions 262321 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r262321 | tilghman | 2010-05-11 12:22:07 -0500 (Tue, 11 May 2010) | 2 lines
   
   Fix issue ASTERISK-16065 a slightly different way (mad props to Qwell)
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262339
By: Digium Subversion (svnbot) 2010-05-11 12:25:28

Repository: asterisk
Revision: 262340

_U  branches/1.6.2/
U   branches/1.6.2/Makefile.rules
U   branches/1.6.2/apps/app_voicemail.c

------------------------------------------------------------------------
r262340 | tilghman | 2010-05-11 12:25:28 -0500 (Tue, 11 May 2010) | 16 lines

Merged revisions 262330 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r262330 | tilghman | 2010-05-11 12:23:51 -0500 (Tue, 11 May 2010) | 9 lines
 
 Merged revisions 262321 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r262321 | tilghman | 2010-05-11 12:22:07 -0500 (Tue, 11 May 2010) | 2 lines
   
   Fix issue ASTERISK-16065 a slightly different way (mad props to Qwell)
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=262340