Bug 210323 - Add WARN_UNUSED_RETURN to decode methods in Source/WTF
Summary: Add WARN_UNUSED_RETURN to decode methods in Source/WTF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-09 22:37 PDT by David Kilzer (:ddkilzer)
Modified: 2020-04-11 17:09 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (4.78 KB, patch)
2020-04-10 02:56 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-04-09 22:37:03 PDT
Add WARN_UNUSED_RETURN to decode methods in Source/WTF.

$ ./Tools/Scripts/check-webkit-style --filter=-,+security/missing_warn_unused_return Source/WTF
ERROR: Source/WTF/wtf/URL.h:195:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
ERROR: Source/WTF/wtf/URL.h:196:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
ERROR: Source/WTF/wtf/MediaTime.h:138:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
ERROR: Source/WTF/wtf/Seconds.h:232:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
ERROR: Source/WTF/wtf/Seconds.h:242:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
ERROR: Source/WTF/wtf/MonotonicTime.h:149:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
ERROR: Source/WTF/wtf/MonotonicTime.h:159:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
ERROR: Source/WTF/wtf/ObjectIdentifier.h:73:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
ERROR: Source/WTF/wtf/persistence/PersistentCoder.h:41:  decode() function returning a value is missing WARN_UNUSED_RETURN attribute  [security/missing_warn_unused_return] [5]
Total errors found: 9 in 819 files
Comment 1 Radar WebKit Bug Importer 2020-04-09 22:37:11 PDT
<rdar://problem/61565764>
Comment 2 David Kilzer (:ddkilzer) 2020-04-10 02:55:05 PDT
No issues found.  This is to prevent future bugs from being introduced.
Comment 3 David Kilzer (:ddkilzer) 2020-04-10 02:56:06 PDT
Created attachment 396068 [details]
Patch v1
Comment 4 Darin Adler 2020-04-10 08:49:42 PDT
Comment on attachment 396068 [details]
Patch v1

For the functions that return Optional, WARN_UNUSED_RETURN has far less utility than for the functions that return bool. Not even sure it’s worthwhile.
Comment 5 David Kilzer (:ddkilzer) 2020-04-10 09:01:45 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 396068 [details]
> Patch v1
> 
> For the functions that return Optional, WARN_UNUSED_RETURN has far less
> utility than for the functions that return bool. Not even sure it’s
> worthwhile.

I have yet to find any cases where the Optional<> values are not already checked, though.  (Maybe that's because the output is always assigned to another Optional<> variable?)

I'll take a look at how they're used.
Comment 6 Darin Adler 2020-04-10 09:19:48 PDT
(In reply to David Kilzer (:ddkilzer) from comment #5)
> I have yet to find any cases where the Optional<> values are not already
> checked, though.  (Maybe that's because the output is always assigned to
> another Optional<> variable?)

My point exactly. It’s very hard to make the mistake of not using the Optional<> because of the design of Optional.

It’s very easy to make the mistake of not using the bool, which is why this is important.
Comment 7 EWS 2020-04-10 09:22:40 PDT
Committed r259878: <https://trac.webkit.org/changeset/259878>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396068 [details].
Comment 8 Alex Christensen 2020-04-10 11:44:08 PDT
I don't think we should add WARN_UNUSED_RETURN to decoders returning Optional.
I also don't think we should go through effort to add WARN_UNUSED_RETURN to existing decoders that return a bool.  That effort should be used to make decoders that return Optional instead.
Comment 9 Darin Adler 2020-04-10 12:16:27 PDT
(In reply to Alex Christensen from comment #8)
> I don't think we should add WARN_UNUSED_RETURN to decoders returning
> Optional.

I agree.

> I also don't think we should go through effort to add WARN_UNUSED_RETURN to
> existing decoders that return a bool.  That effort should be used to make
> decoders that return Optional instead.

I don’t agree.

The effort to refactor from bool to Optional is greater than the effort to add the warning. And the warning protects us until we refactor.
Comment 10 Alex Christensen 2020-04-10 18:05:31 PDT
Could we at least land https://bugs.webkit.org/show_bug.cgi?id=207497 before doing much more IPC/encoding stuff?
Comment 11 David Kilzer (:ddkilzer) 2020-04-10 18:57:50 PDT
(In reply to Alex Christensen from comment #10)
> Could we at least land https://bugs.webkit.org/show_bug.cgi?id=207497 before
> doing much more IPC/encoding stuff?

Sure.
Comment 12 David Kilzer (:ddkilzer) 2020-04-10 18:59:34 PDT
(In reply to David Kilzer (:ddkilzer) from comment #11)
> (In reply to Alex Christensen from comment #10)
> > Could we at least land https://bugs.webkit.org/show_bug.cgi?id=207497 before
> > doing much more IPC/encoding stuff?
> 
> Sure.

Except I just landed this follow-up fix for the suggestions you and Darin made:

Committed r259917: <https://trac.webkit.org/changeset/259917>

Sorry about that.  Didn't see this until after I landed the change.