This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add option `RewriteVoidReturnType` to `modernize-use-trailing-return-type`
Needs ReviewPublic

Authored by Izaron on Oct 12 2022, 3:53 PM.

Details

Summary

The check doesn't trigger for functions returning void. The new options
is added to allow to have a complete homogeneous code.

Fixes https://github.com/llvm/llvm-project/issues/54383

Diff Detail

Event Timeline

Izaron created this revision.Oct 12 2022, 3:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 3:53 PM
Izaron requested review of this revision.Oct 12 2022, 3:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 3:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Izaron added inline comments.Oct 12 2022, 3:55 PM
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
398–400

ctors/dtors are voidType, so we need to add filters for them when removing filter for voidType

Hi! I am the original author of this check. I very much welcome your contribution! Thank you for the effort!

I am not a clang tools maintainer though, so you will need someone else to review and approve this change set.

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
420–421

I would intuitively check the cheaper condition first to benefit form short-circuit evaluation.

clang-tools-extra/docs/clang-tidy/checks/modernize/use-trailing-return-type.rst
73–76

I picked up the habit to word conditions/options in a positive manner. So I would rather name the option RewriteVoidReturnType instead of IgnoreVoidReturnType and use the inverted Booleans throughout the code. The reason is that my brain parses e.g. if (!RewriteVoidReturnType) easierly than if(!IgnoreVoidReturnType) because of the double negation.

Feel free to debate me here. This is not a strong opinion.

Izaron updated this revision to Diff 467590.Oct 13 2022, 1:42 PM

Change IgnoreVoidReturnType to RewriteVoidReturn type.
Check the cheaper condition first.

A big thank to @bernhardmgruber for nice comments!

Izaron retitled this revision from [clang-tidy] Add option `IgnoreVoidReturnType` to `modernize-use-trailing-return-type` to [clang-tidy] Add option `RewriteVoidReturnType` to `modernize-use-trailing-return-type`.Oct 13 2022, 1:42 PM
Izaron marked 2 inline comments as done.

The default behaviour of this check should be transform void return types as that's how it has been since the check was first created. Adding an option which defaults to changing this behaviour would be harmful to current users of this check.

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
132

There's no reason this should be accessible globally.

clang-tools-extra/docs/clang-tidy/checks/modernize/use-trailing-return-type.rst
73–76

Please change this back to IgnoreVoidReturnType or maybe even IgnoreVoid, A lot of checks use Ignore options to help restrict what the check will report on, so we should follow that convention.
If you want to avoid the double negatives that can be achieved by negative the value when you read and write the option from/to the options map.

RewriteVoidReturnType(!Options.get("IgnoreVoid", false))
LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:17 AM