Page MenuHomePhabricator

[clang-tidy] modernize-loop-convert: limit use of auto
Needs ReviewPublic

Authored by eddy-geek on May 26 2021, 10:35 AM.

Details

Summary

Fixes bug 35694
Adds option AutoTypeNameLength, default 0 (keep current behaviour).
Note that the default could be set to 5 in the future
(like for modernize-use-auto.MinTypeNameLength).

Diff Detail

Event Timeline

eddy-geek created this revision.May 26 2021, 10:35 AM
eddy-geek requested review of this revision.May 26 2021, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 10:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I haven't been able to build locally so far -- OOM on my machine, not sure if I can do something lighter than:

cmake -G Ninja -DLLVM_ENABLE_PROJECTS=clang\;clang-tools-extra -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=$PWD -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ../llvm
ninja check-clang-tools

but I 'm still looking for feedback in the meantime.

Fix test build

Fix test output

*Trigger rebuild (windows failed to git clone)*

eddy-geek updated this revision to Diff 348080.May 26 2021, 1:48 PM

*Trigger rebuild (windows failed to git clone)*

eddy-geek updated this revision to Diff 348081.May 26 2021, 1:51 PM

*Trigger rebuild (windows failed to git clone)*

eddy-geek added a subscriber: sammccall.EditedMay 27 2021, 8:12 AM

Builds ok, ready for review. I can't edit reviewers now, can someone help? @sammccall / @njames93 / @Eugene.Zelenko / @Mordante ?

Frankly, I don't think that length-based criteria is reasonable one. Readability of code is much more important than line/file length.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
633

Please don't use auto unless type is explicitly spelled in same statement or iterator.

clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
163

See other documentation file for syntax for options.

166

Please highlight TypeName (option) is single back-ticks and auto (language construct) with double back-ticks.

168

Please highlight 0 (option value) is single back-ticks and auto (language construct) with double back-ticks.

eddy-geek added a subscriber: Eugene.Zelenko.EditedJun 1 2021, 10:16 AM

Frankly, I don't think that length-based criteria is reasonable one. Readability of code is much more important than line/file length.

It provides an added configuration flexibility without being more complex than a boolean. Arguably some people have found the similar modernize-use-auto.MinTypeNameLength useful (the PR has some discussion) -- and I find its default of 5 makes sense as MyTyp can be argued as always better than auto even for people that like auto.

from @alexfh on that thread:

the style guides I'm more or less familiar with recommend the use of auto for "long/cluttery type names" [1], "if and only if it makes the code more readable or easier to maintain" [2], or to "save writing a longish, hard-to-remember type that the compiler already knows but a programmer could get wrong" [3]. None of these guidelines seem to endorse the use of auto instead of int, bool or the like.
From my perspective, the default value of 5 would cover a larger fraction of real-world use cases.

That said, I absolutely don't mind changing this PR to define a boolean option UseAuto instead, if requested.

(otherwise I'll address review comments tomorrow. Thanks for the review!)