Bug 135889 – Clean up build warnings: control reaches end of non-void function

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Clean up build warnings: control reaches end of non-void function

[Yusuke Suzuki]

Created attachment 236535 [details]
Patch

[Yusuke Suzuki]

Created attachment 236536 [details]
Patch

[Alexey Proskuryakov]

What compiler complains about this? These are strongly type enums, so I don't see how this is a valid warning. Perhaps it should be disabled instead?

[Yusuke Suzuki]

(In reply to comment #3)
> What compiler complains about this? These are strongly type enums, so I don't see how this is a valid warning. Perhaps it should be disabled instead?

Compiler version is gcc 4.9.1, that's a default compiler for Ubuntu 14.04.

[Alexey Proskuryakov]

What do you think about disabling this warning? It seems clearly invalid to me.

[Benjamin Poulain]

(In reply to comment #5)
> What do you think about disabling this warning? It seems clearly invalid to me.

We decided last year not to disable the warning for GCC. See the thread 'Can we disable "control reaches end of non-void function" warning on Qt?' on webkit-dev.

[Alexey Proskuryakov]

Wasn't that discussion about old-style enums, not strongly typed enums?

[Yusuke Suzuki]

(In reply to comment #7)
> Wasn't that discussion about old-style enums, not strongly typed enums?

Reading the thread, maybe that's correct.
My concern is that disabling this warning is not limited for strongly-typed enums. So personally I think disabling this causes issues when some non-strongly-typed enum value is appended to the existing enums. But I'd like to hear Alexey and Benjamin's opinions :)

[Alexey Proskuryakov]

I don't have a strong opinion. Just like to keep the code clean.

[Oliver Hunt]

Comment on attachment 236536 [details]
Patch

Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED()

[Yusuke Suzuki]

(In reply to comment #10)
> (From update of attachment 236536 [details])
> Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED()

Thanks for your review. That's reasonable and safe.
I'll change to it and update the patch.

[Yusuke Suzuki]

Created attachment 236787 [details]
Patch

[Benjamin Poulain]

I am okay with this.

Alexey?

[Yusuke Suzuki]

(In reply to comment #13)
> I am okay with this.
> 
> Alexey?

Oh! I've missed Alexey's r+. Thank you for the discussion Alexey and Benjamin. And thx for your review, Oliver.
I'll land it :)

[WebKit Commit Bot]

Comment on attachment 236787 [details]
Patch

Clearing flags on attachment: 236787

Committed r172886: <http://trac.webkit.org/changeset/172886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.