unreachable "return this" in TIntermConstantUnion::fold

[GoogleCodeExporter]

What steps will reproduce the problem?
1. run Coverity on mozilla-central from something after 
http://hg.mozilla.org/mozilla-central/rev/582be9aca672

What is the expected output?
no DEADCODE warnings for unreachable code in TIntermConstantUnion::fold

What do you see instead?
DEADCODE warning in TIntermConstantUnion::fold for "return this"

What version of the product are you using?
r367

On what operating system?
Linux with Coverity 4.5

Please provide any additional information below.
Short of using -U300 with svn there's no easy way to show that this change is 
correct. Instead of doing that, attached is a patch which refactors the code by 
moving the else case first and dropping the } else { after its return.

Original issue reported on code.google.com by timel...@gmail.com on 12 Oct 2010 at 4:13

Attachments:

• angleproject-TIntermConstantUnion-fold

[GoogleCodeExporter]

Some compilers may actually generate a warning or even an error without the 
unreachable line, thinking the function does not return something on all paths 
due to incorrect/incomplete analysis. In fact static analysis of unreachable 
code is never complete. It is sometimes mathematically provable that some code 
is unreachable, but most tools won't catch the advanced cases.

It can also be valuable to know what the safe fallthrough value is supposed to 
be, should it ever be needed. So sometimes unreachable code can be as valuable 
for code maintenance as comments.

I haven't actually looked at this particular case in detail, but I'd just like 
to warn against blindly using tools like Coverity for indicating things that 
need 'fixing'. Changing the code may actually cause more issues. So while these 
warnings can be very valuable when they point out serious flaws, for some of 
them it can be a matter of opinion whether they justify taking action.

Original comment by Nicolas....@gmail.com on 12 Oct 2010 at 10:09

[GoogleCodeExporter]

While I agree with Nicolas, I will look into this particular case if the 
warning can be easily eliminated.

Original comment by alokp@chromium.org on 14 Oct 2010 at 6:46

• Changed state: Accepted

[GoogleCodeExporter]

Does anyone still care about this, or do we agree not to take action on it?

Original comment by c...@chromium.org on 31 Jan 2014 at 6:48

[GoogleCodeExporter]

I provided a patch. I'm interested in hearing why it wasn't looked at in 2010...

Original comment by timel...@gmail.com on 24 Feb 2014 at 2:08

[GoogleCodeExporter]

Your patch wasn't ignored, but there was a floating question over whether it 
was worthwhile to shuffle code to quiet a static analysis warning in this case. 
It looks like the answer did become "yes," at least as far as eliminating the 
unreachable line, as the return in question was eliminated in 2012 as part of a 
larger patch that addressed MSVC warnings: 
https://codereview.appspot.com/5570066

Closing as fixed; if this is still an issue for some reason I've missed, please 
feel free to refile.

Original comment by shannonw...@chromium.org on 24 Feb 2014 at 8:27

• Changed state: Fixed