Bug 239290 - Misc compiler warnings, April 2022 edition
Summary: Misc compiler warnings, April 2022 edition
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-13 11:50 PDT by Michael Catanzaro
Modified: 2022-04-19 10:20 PDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2022-04-13 11:50:18 PDT
Misc compiler warnings, April 2022 edition

I'm starting to get tired of -Wreturn-type....
Comment 1 Michael Catanzaro 2022-04-13 12:21:03 PDT
https://github.com/WebKit/WebKit/pull/282
Comment 2 EWS 2022-04-13 15:56:15 PDT
Committed r292840 (249613@main): <https://commits.webkit.org/249613@main>

Reviewed commits have been landed. Closing PR #282 and removing active labels.
Comment 3 Radar WebKit Bug Importer 2022-04-13 15:57:15 PDT
<rdar://problem/91720326>
Comment 4 Yusuke Suzuki 2022-04-19 09:39:59 PDT
Please do not attach RELEASE_ASSERT_NOT_REACHED here. Some of them are super hot critical function, and it hurts our performance. Use 269IGNORE_RETURN_TYPE_WARNINGS_BEGIN
Comment 5 Yusuke Suzuki 2022-04-19 09:45:31 PDT
Committed r293018 (249757@trunk): <https://commits.webkit.org/249757@trunk>
Comment 6 Michael Catanzaro 2022-04-19 09:59:52 PDT
ack, thanks for catching it! How scary. :(

It will happen again in the future, though, because I can't know where these areas may be, and we keep introducing more and more return type warnings. These make up probably >50% of the GCC warnings that I deal with.

One option is to give up and build with -Wno-return-type, because fixing up switch statements is getting pretty old. But sadly, it does seem like a useful warning except for this pattern with switch statements, so I'm a bit hesitant to do that.
Comment 7 Yusuke Suzuki 2022-04-19 10:02:59 PDT
(In reply to Michael Catanzaro from comment #6)
> ack, thanks for catching it! How scary. :(
> 
> It will happen again in the future, though, because I can't know where these
> areas may be, and we keep introducing more and more return type warnings.
> These make up probably >50% of the GCC warnings that I deal with.
> 
> One option is to give up and build with -Wno-return-type, because fixing up
> switch statements is getting pretty old. But sadly, it does seem like a
> useful warning except for this pattern with switch statements, so I'm a bit
> hesitant to do that.

Can GCC have a mechanism like this? I think it can suppress most of false positives.

1. emitting this warning for normal enum case
2. not emitting this for strongly scoped enum when switch handles all scoped enums
Comment 8 Michael Catanzaro 2022-04-19 10:09:10 PDT
GCC does have many options (-Wswitch, -Wswitch-default, and -Wswitch-enum) to get exactly your preferred warnings for missing switch cases, but -Wreturn-type is orthogonal to those.
Comment 9 Michael Catanzaro 2022-04-19 10:15:30 PDT
Did some searching... it seems C++23 has a solution for this, std::unreachable():

https://en.cppreference.com/w/cpp/utility/unreachable

In the meantime, we could add a WTF_UNREACHABLE() macro fairly easily, which could replace most use of RELEASE_ASSERT_NOT_REACHED() after switch statements that are intended to always return. Hopefully that would allow us to avoid the performance hit in these locations? Then we wouldn't have to worry about this happening again. (I know it takes you ages to track it down performance regressions....)
Comment 10 Michael Catanzaro 2022-04-19 10:20:20 PDT
I see we already have UNREACHABLE_FOR_PLATFORM in Assertions.h, but it just calls RELEASE_ASSERT_NOT_REACHED() with a warning not to change that. <_<