| Summary: | Clean up build warnings: control reaches end of non-void function | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ap, benjamin, commit-queue, darin, ysuzuki | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Yusuke Suzuki
2014-08-13 11:07:53 PDT
Created attachment 236535 [details]
Patch
Created attachment 236536 [details]
Patch
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? (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. What do you think about disabling this warning? It seems clearly invalid to me. (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. Wasn't that discussion about old-style enums, not strongly typed enums? (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 :) I don't have a strong opinion. Just like to keep the code clean. Comment on attachment 236536 [details]
Patch
Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED()
(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. Created attachment 236787 [details]
Patch
I am okay with this. Alexey? (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 :) Comment on attachment 236787 [details] Patch Clearing flags on attachment: 236787 Committed r172886: <http://trac.webkit.org/changeset/172886> All reviewed patches have been landed. Closing bug. |