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

[eddy-geek]

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

[eddy-geek]

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.

[eddy-geek]

Fix test build

[eddy-geek]

Fix test output

[eddy-geek]

*Trigger rebuild (windows failed to git clone)*

[eddy-geek]

*Trigger rebuild (windows failed to git clone)*

[eddy-geek]

*Trigger rebuild (windows failed to git clone)*

[eddy-geek]

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

[Eugene.Zelenko]

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]

@Eugene.Zelenko wrote:

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!)

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h

bool isConvertible(ASTContext *Context, const ast_matchers::BoundNodes &Nodes,
                      const ForStmt *Loop, LoopFixerKind FixerKind);
 
   StringRef getReverseFunction() const;
   StringRef getReverseHeader() const;
 
   std::unique_ptr<TUTrackingInfo> TUInfo;
   const unsigned long long MaxCopySize;
   const Confidence::Level MinConfidence;
+  const unsigned int AutoTypeNameLength;
   const VariableNamer::NamingStyle NamingStyle;
   utils::IncludeInserter Inserter;
   bool UseReverseRanges;
   const bool UseCxx20IfAvailable;
   std::string ReverseFunction;
   std::string ReverseHeader;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H

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

 LoopConvertCheck::RangeDescriptor::RangeDescriptor()
     : ContainerNeedsDereference(false), DerefByConstRef(false),
       DerefByValue(false), NeedsReverseCall(false) {}
 
 LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
       MaxCopySize(Options.get("MaxCopySize", 16ULL)),
       MinConfidence(Options.get("MinConfidence", Confidence::CL_Reasonable)),
+      AutoTypeNameLength(Options.get("AutoTypeNameLength", 0)),
       NamingStyle(Options.get("NamingStyle", VariableNamer::NS_CamelCase)),
       Inserter(Options.getLocalOrGlobal("IncludeStyle",
                                         utils::IncludeSorter::IS_LLVM)),
       UseCxx20IfAvailable(Options.get("UseCxx20ReverseRanges", true)),
       ReverseFunction(Options.get("MakeReverseRangeFunction", "")),
       ReverseHeader(Options.get("MakeReverseRangeHeader", "")) {
 
   if (ReverseFunction.empty() && !ReverseHeader.empty()) {
     configurationDiag(
         "modernize-loop-convert: 'MakeReverseRangeHeader' is set but "
         "'MakeReverseRangeFunction' is not, disabling reverse loop "
         "transformation");
     UseReverseRanges = false;
   } else if (ReverseFunction.empty()) {
     UseReverseRanges = UseCxx20IfAvailable && getLangOpts().CPlusPlus20;
   } else {
     UseReverseRanges = true;
   }
 }
 
 void LoopConvertCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "MaxCopySize", MaxCopySize);
   Options.store(Opts, "MinConfidence", MinConfidence);
+  Options.store(Opts, "AutoTypeNameLength", AutoTypeNameLength);
   Options.store(Opts, "NamingStyle", NamingStyle);
   Options.store(Opts, "IncludeStyle", Inserter.getStyle());
   Options.store(Opts, "UseCxx20ReverseRanges", UseCxx20IfAvailable);
   Options.store(Opts, "MakeReverseRangeFunction", ReverseFunction);
   Options.store(Opts, "MakeReverseRangeHeader", ReverseHeader);
 }
 
 void LoopConvertCheck::registerPPCallbacks(const SourceManager &SM,
for (const auto &Usage : Usages) {
           CharSourceRange::getTokenRange(Range), ReplaceText));
     }
   }
 
   // Now, we need to construct the new range expression.
   SourceRange ParenRange(Loop->getLParenLoc(), Loop->getRParenLoc());
 
   QualType Type = Context->getAutoDeductType();
-  if (!Descriptor.ElemType.isNull() && Descriptor.ElemType->isFundamentalType())
-    Type = Descriptor.ElemType.getUnqualifiedType();
+  if (!Descriptor.ElemType.isNull()) {
+    auto FullType = Descriptor.ElemType.getUnqualifiedType();
+    if (Descriptor.ElemType->isFundamentalType() ||
+        FullType.getAsString(getLangOpts()).length() <= AutoTypeNameLength)
+      Type = FullType;
+  }
   Type = Type.getDesugaredType(*Context);
 
   // If the new variable name is from the aliased variable, then the reference
   // type for the new variable should only be used if the aliased variable was
   // declared as a reference.
   bool IsCheapToCopy =
       !Descriptor.ElemType.isNull() &&
       Descriptor.ElemType.isTriviallyCopyableType(*Context) &&

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

.. code-block:: c++
 
   // reasonable conversion
   for (auto & elem : v)
     cout << elem;
 
 Reverse Iterator Support
 ------------------------
 
-The converter is also capable of transforming iterator loops which use 
-``rbegin`` and ``rend`` for looping backwards over a container. Out of the box 
-this will automatically happen in C++20 mode using the ``ranges`` library, 
-however the check can be configured to work without C++20 by specifying a 
+The converter is also capable of transforming iterator loops which use
+``rbegin`` and ``rend`` for looping backwards over a container. Out of the box
+this will automatically happen in C++20 mode using the ``ranges`` library,
+however the check can be configured to work without C++20 by specifying a
 function to reverse a range and optionally the header file where that function
 lives.
 
 .. option:: UseCxx20ReverseRanges
-  
-   When set to true convert loops when in C++20 or later mode using 
+   When set to true convert loops when in C++20 or later mode using
    ``std::ranges::reverse_view``.
    Default value is ``true``.
 
 .. option:: MakeReverseRangeFunction
 
-   Specify the function used to reverse an iterator pair, the function should 
-   accept a class with ``rbegin`` and ``rend`` methods and return a 
+   Specify the function used to reverse an iterator pair, the function should
+   accept a class with ``rbegin`` and ``rend`` methods and return a
    class with ``begin`` and ``end`` methods methods that call the ``rbegin`` and
    ``rend`` methods respectively. Common examples are ``ranges::reverse_view``
    and ``llvm::reverse``.
    Default value is an empty string.
 
 .. option:: MakeReverseRangeHeader
 
    Specifies the header file where :option:`MakeReverseRangeFunction` is
-   declared. For the previous examples this option would be set to 
+   declared. For the previous examples this option would be set to
    ``range/v3/view/reverse.hpp`` and ``llvm/ADT/STLExtras.h`` respectively.
-   If this is an empty string and :option:`MakeReverseRangeFunction` is set, 
-   the check will proceed on the assumption that the function is already 
+   If this is an empty string and :option:`MakeReverseRangeFunction` is set,
+   the check will proceed on the assumption that the function is already
    available in the translation unit.
    This can be wrapped in angle brackets to signify to add the include as a
    system include.
    Default value is an empty string.
 
 .. option:: IncludeStyle
 
    A string specifying which include-style is used, `llvm` or `google`. Default
    is `llvm`.
 
+AutoTypeNameLength option
+------------------------
+
+If TypeName length is strictly above this threshold, `auto` will be used.
+
+The default is 0, so `auto` will be used for all types except C++ fundamental types.
+
 Limitations
 -----------
 
 There are certain situations where the tool may erroneously perform
 transformations that remove information and change semantics. Users of the tool
 should be aware of the behaviour and limitations of the check outlined by
 the cases below.
 

clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-auto.cpp

+// RUN: %check_clang_tidy %s modernize-loop-convert %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-loop-convert.AutoTypeNameLength, value: '22'}]}" \
+// RUN:   -- -I %S/Inputs/modernize-loop-convert
+
+#include "structures.h"
+
+const int N = 6;
+
+void autotype() {
+  Val Teas[N];
+  for (int I = 0; I < N; ++I) {
+    Teas[I].g();
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (Val & Tea : Teas)
+  // CHECK-FIXES-NEXT: Tea.g();
+
+  // NonTriviallyCopyable has length 21 < 22
+  const NonTriviallyCopyable NonCopy[N]{};
+  for (int I = 0; I < N; ++I) {
+    printf("2 * %d = %d\n", NonCopy[I].X, NonCopy[I].X + NonCopy[I].X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (const NonTriviallyCopyable & I : NonCopy)
+  // CHECK-FIXES-NEXT: printf("2 * %d = %d\n", I.X, I.X + I.X);
+
+  // TriviallyCopyableButBig has length 23 > 22
+  const TriviallyCopyableButBig Big[N]{};
+  for (int I = 0; I < N; ++I) {
+    printf("2 * %d = %d\n", Big[I].X, Big[I].X + Big[I].X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (const auto & I : Big)
+  // CHECK-FIXES-NEXT: printf("2 * %d = %d\n", I.X, I.X + I.X);
+}