Bug 210336 – check-webkit-style should warn about 'decode' functions with missing WARN_UNUSED_RETURN attribute

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

check-webkit-style should warn about 'decode' functions that are missing the WARN_UNUSED_RETURN attribute.

This will help to find bugs when using CoreIPC decode() methods.

[Radar WebKit Bug Importer]

<rdar://problem/61585835>

[David Kilzer (:ddkilzer)]

Created attachment 396088 [details]
Patch v1

[Darin Adler]

Comment on attachment 396088 [details]
Patch v1

For functions returning bool I think this is a good rule. For ones returning Optional I think this is not helpful, but I suppose no big deal to comply anyway.

[Jonathan Bedard]

Comment on attachment 396088 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=396088&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1818
> +            if function_state.has_return_type('(auto|bool|Optional<.+>)'):

I would actually expect this to be an inverse check. If a function does not have a void return type, then we should trigger this logic. Any reason we picked this set of types?

[Jonathan Bedard]

Comment on attachment 396088 [details]
Patch v1

Darin r+ed it, I accidentally removed the r+ when adding a comment

[Darin Adler]

If we are going to consider return types, the real issue is about bool return types. I don’t see a real unused return value issue outside of the bool return type case.

[Alex Christensen]

Comment on attachment 396088 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=396088&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:3039
> +            '    static WARN_UNUSED_RETURN Optional<std::tuple<>> decode(Decoder&)\n'

This shouldn't be added.

[David Kilzer (:ddkilzer)]

Comment on attachment 396088 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=396088&action=review

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1818
>> +            if function_state.has_return_type('(auto|bool|Optional<.+>)'):
> 
> I would actually expect this to be an inverse check. If a function does not have a void return type, then we should trigger this logic. Any reason we picked this set of types?

This set of types are specific to the decode() methods we care about.

I'm going to remove Optional<.+> from the list per feedback from Alex and Darin.

>> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:3039
>> +            '    static WARN_UNUSED_RETURN Optional<std::tuple<>> decode(Decoder&)\n'
> 
> This shouldn't be added.

I will remove "Optional<.+>" from the list of types based on feedback from you and Darin, and remove this test (since it's a dupe of the one above) before landing.

[David Kilzer (:ddkilzer)]

Created attachment 396158 [details]
Patch for landing

[EWS]

Committed r259925: <https://trac.webkit.org/changeset/259925>

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