[clang-tidy]bugprone-fold-init-type

[courbet]

Summary

Handle iterators that do not define a value_type type alias.

Get the value type from the return type of Iter::operator* instead.

[courbet]

fix typo

[Eugene.Zelenko]

clang-tools-extra/test/clang-tidy/checkers/bugprone/fold-init-type.cpp

205

Excessive newline.

[courbet]

remove extra training newline

[courbet]

ping

[courbet]

ping

[PiotrZSL]

Missing ReleaseNotes,

clang-tools-extra/clang-tidy/bugprone/FoldInitTypeCheck.cpp

39

referenceType(pointee -> references

41

BuiltinTypeWithId already got hasCanonicalType, no need to duplicate

43

maybe:

returns(qualType(hasCanonicalType(anyOf(BuiltinTypeWithId(ID), references(BuiltinTypeWithId(ID)), autoType(hasDeducedType(BuiltinTypeWithId(ID)))

instead of having anyOf on top level.

[courbet]

• Address review comments
• Add release notes.

Thanks !

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

101

Wrong section, this should be in 'Changes in existing checks'

[courbet]

Fix release note placement and phrase it more consistenly with other notes in that section.

[PiotrZSL]

LGTM

[courbet]

Thanks for the review.

[Eugene.Zelenko]

Please rebase from latest main.

clang-tools-extra/docs/ReleaseNotes.rst

294

Please keep alphabetical order (by check name) in this section.

[courbet]

clang-tools-extra/docs/ReleaseNotes.rst

294

Done (in d880d4e7c228).

clang-tools-extra/clang-tidy/bugprone/FoldInitTypeCheck.cpp

void FoldInitTypeCheck::registerMatchers(MatchFinder *Finder) {
   // Note: Right now we check only builtin types.
   const auto BuiltinTypeWithId = [](const char *ID) {
     return hasCanonicalType(builtinType().bind(ID));
   };
   const auto IteratorWithValueType = [&BuiltinTypeWithId](const char *ID) {
     return anyOf(
         // Pointer types.
         pointsTo(BuiltinTypeWithId(ID)),
-        // Iterator types.
-        recordType(hasDeclaration(has(typedefNameDecl(
-            hasName("value_type"), hasType(BuiltinTypeWithId(ID)))))));
+        // Iterator types have an `operator*` whose return type is the type we
+        // care about.
+        // Notes:
+        //   - `operator*` can be in one of the bases of the iterator class.
+        //   - this does not handle cases when the `operator*` is defined
+        //     outside the iterator class.
+        recordType(
+            hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom(has(functionDecl(
+                hasOverloadedOperatorName("*"),
+                returns(qualType(hasCanonicalType(anyOf(
+                    // `value_type& operator*();`
+                    references(BuiltinTypeWithId(ID)),
+                    // `value_type operator*();`
+                    BuiltinTypeWithId(ID),
+                    // `auto operator*();`, `decltype(auto) operator*();`
+                    autoType(hasDeducedType(BuiltinTypeWithId(ID)))
+                    //
+                    )))))))))));
   };
 
   const auto IteratorParam = parmVarDecl(
       hasType(hasCanonicalType(IteratorWithValueType("IterValueType"))));
   const auto Iterator2Param = parmVarDecl(
       hasType(hasCanonicalType(IteratorWithValueType("Iter2ValueType"))));
   const auto InitParam = parmVarDecl(hasType(BuiltinTypeWithId("InitType")));
 

clang-tools-extra/docs/ReleaseNotes.rst

 
 Improvements to clang-tidy
 --------------------------
 
 - New global configuration file options `HeaderFileExtensions` and
   `ImplementationFileExtensions`, replacing the check-local options of the
   same name.
 
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check.
 
   Checks for functions that have safer, more secure replacements available, or
   are considered deprecated due to design flaws.
   This check relies heavily on, but is not exclusive to, the functions from

 - Fixed an issue in :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name>` when using
   ``DISABLED_`` in the test suite name.
 
 - Fixed a false positive in :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
+- Improved :doc:`bugprone-fold-init-type
+  <clang-tidy/checks/bugprone/fold-init-type>` to handle iterators that do not
+  define `value_type` type aliases.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/fold-init-type.cpp

-// RUN: %check_clang_tidy %s bugprone-fold-init-type %t
+// RUN: %check_clang_tidy %s bugprone-fold-init-type -std=c++17 %t
 
 namespace std {
 template <class InputIt, class T>
-T accumulate(InputIt first, InputIt last, T init);
+T accumulate(InputIt first, InputIt last, T init) {
+  // When `InputIt::operator*` returns a deduced `auto` type that refers to a
+  // dependent type, the return type is deduced only if `InputIt::operator*`
+  // is instantiated. In practice this happens somewhere in the implementation
+  // of `accumulate`. For tests, do it here.
+  (void)*first;
+}
 
 template <class InputIt, class T>
-T reduce(InputIt first, InputIt last, T init);
+T reduce(InputIt first, InputIt last, T init) { (void)*first; }
 template <class ExecutionPolicy, class InputIt, class T>
 T reduce(ExecutionPolicy &&policy,
-         InputIt first, InputIt last, T init);
+         InputIt first, InputIt last, T init) { (void)*first; }
 
 struct parallel_execution_policy {};
 constexpr parallel_execution_policy par{};
 
 template <class InputIt1, class InputIt2, class T>
 T inner_product(InputIt1 first1, InputIt1 last1,
-                InputIt2 first2, T value);
+                InputIt2 first2, T value) { (void)*first1; (void)*first2; }
 
 template <class ExecutionPolicy, class InputIt1, class InputIt2, class T>
 T inner_product(ExecutionPolicy &&policy, InputIt1 first1, InputIt1 last1,
-                InputIt2 first2, T value);
+                InputIt2 first2, T value) { (void)*first1; (void)*first2; }
 
 } // namespace std
 
 struct FloatIterator {
-  typedef float value_type;
+  const float &operator*() const;
 };
+
+struct DerivedFloatIterator : public FloatIterator {
+};
+
+template <typename ValueType> struct ByValueTemplateIterator {
+  ValueType operator*() const;
+};
+
+template <typename ValueType> struct ByRefTemplateIterator {
+  ValueType &operator*();
+};
+
+template <typename ValueType> struct ByRefTemplateIteratorWithAlias {
+  using reference = const ValueType&;
+  reference operator*();
+};
+
+template <typename ValueType> struct AutoByValueTemplateIterator {
+  auto operator*() const { return ValueType{}; }
+};
+
+template <typename ValueType> struct AutoByRefTemplateIterator {
+  decltype(auto) operator*() const { return value_; }
+  ValueType value_;
+};
+
 template <typename ValueType>
-struct TypedefTemplateIterator { typedef ValueType value_type; };
-template <typename ValueType>
-struct UsingTemplateIterator { using value_type = ValueType; };
-template <typename ValueType>
-struct DependentTypedefTemplateIterator { typedef typename ValueType::value_type value_type; };
-template <typename ValueType>
-struct DependentUsingTemplateIterator : public TypedefTemplateIterator<ValueType> { using typename TypedefTemplateIterator<ValueType>::value_type; };
+struct InheritingByConstRefTemplateIterator
+    : public ByRefTemplateIterator<const ValueType> {};
+
 using TypedeffedIterator = FloatIterator;
 
 // Positives.
 
 int accumulatePositive1() {
   float a[1] = {0.5f};
   return std::accumulate(a, a + 1, 0);
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int'
 }
 
 int accumulatePositive2() {
   FloatIterator it;
   return std::accumulate(it, it, 0);
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int'
 }
 
 int accumulatePositive3() {
+  DerivedFloatIterator it;
+  return std::accumulate(it, it, 0);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int'
+}
+
+int accumulatePositive4() {
   double a[1] = {0.0};
   return std::accumulate(a, a + 1, 0.0f);
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'double' into type 'float'
 }
 
-int accumulatePositive4() {
-  TypedefTemplateIterator<unsigned> it;
+int accumulatePositive5() {
+  ByValueTemplateIterator<unsigned> it;
   return std::accumulate(it, it, 0);
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int'
 }
 
-int accumulatePositive5() {
-  UsingTemplateIterator<unsigned> it;
+int accumulatePositive6() {
+  ByRefTemplateIterator<unsigned> it;
   return std::accumulate(it, it, 0);
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int'
 }
 
-int accumulatePositive6() {
-  DependentTypedefTemplateIterator<UsingTemplateIterator<unsigned>> it;
+int accumulatePositive7() {
+  AutoByValueTemplateIterator<unsigned> it;
   return std::accumulate(it, it, 0);
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int'
 }
 
-int accumulatePositive7() {
+int accumulatePositive8() {
   TypedeffedIterator it;
   return std::accumulate(it, it, 0);
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int'
 }
 
-int accumulatePositive8() {
-  DependentUsingTemplateIterator<unsigned> it;
+int accumulatePositive9() {
+  InheritingByConstRefTemplateIterator<unsigned> it;
+  return std::accumulate(it, it, 0);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int'
+}
+
+int accumulatePositive10() {
+  AutoByRefTemplateIterator<unsigned> it;
   return std::accumulate(it, it, 0);
-  // FIXME: this one should trigger too.
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int'
+}
+
+int accumulatePositive11() {
+  ByRefTemplateIteratorWithAlias<unsigned> it;
+  return std::accumulate(it, it, 0);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'unsigned int' into type 'int'
 }
 
 int reducePositive1() {
   float a[1] = {0.5f};
   return std::reduce(a, a + 1, 0);
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: folding type 'float' into type 'int'
 }
 

 
 int negative3() {
   float a[1] = {0.5f};
   // This is OK because the user explicitly specified T.
   return std::accumulate<float *, float>(a, a + 1, 0);
 }
 
 int negative4() {
-  TypedefTemplateIterator<unsigned> it;
+  ByValueTemplateIterator<unsigned> it;
   // For now this is OK.
   return std::accumulate(it, it, 0.0);
 }
 
 int negative5() {
   float a[1] = {0.5f};
   float b[1] = {1.0f};
   return std::inner_product(std::par, a, a + 1, b, 0.0f);
 }
 
 namespace blah {
 namespace std {
 template <class InputIt, class T>
 T accumulate(InputIt, InputIt, T); // We should not care about this one.
 }
 
 int negative5() {
   float a[1] = {0.5f};
   // Note that this is using blah::std::accumulate.
   return std::accumulate(a, a + 1, 0);
 }
 }