Bug 210323 – Add WARN_UNUSED_RETURN to decode methods in Source/WTF

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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

[Radar WebKit Bug Importer]

<rdar://problem/61565764>

[David Kilzer (:ddkilzer)]

No issues found.  This is to prevent future bugs from being introduced.

[David Kilzer (:ddkilzer)]

Created attachment 396068 [details]
Patch v1

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

(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.

[Darin Adler]

(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.

[EWS]

Committed r259878: <https://trac.webkit.org/changeset/259878>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396068 [details].

[Alex Christensen]

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.

[Darin Adler]

(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.

[Alex Christensen]

Could we at least land https://bugs.webkit.org/show_bug.cgi?id=207497 before doing much more IPC/encoding stuff?

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.