[clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[JonasToth]

Summary

'misc-const-correctness' previously considered arrays as 'Values' independent of the type of the elements.
This is inconsistent with the configuration of the check to disable treating pointers as values.
This patch rectifies this inconsistency.

Fixes #56749

[JonasToth]

clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp

77

the tests must be exanded to check that typedefs in arrays are handled properly.

[JonasToth]

• add test with typedef
• [docs] improve documentation for misc-const-correctness

[JonasToth]

• improve test

[njames93]

clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp

14

Typically don't need this include.

clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst

7

These documentation changes should be split into their own patch.
Same goes for below

[JonasToth]

• split patch
• remove unnecessary includes

[JonasToth]

• remove bad change from diff

[njames93]

clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp

27–30

Wouldn't this behaviour be expected before the changes added in this patch, as p_local1 isn't an array type.

[JonasToth]

clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp

27–30

the behaviour for p_local1 does not change with the patch and is tested/moved here.

only p_local0 has a behaviour change. this test file activates the analysis for pointers-as-values, so it must be diagnosed in this test file.
in the other test-file -values.cpp p_local0 is not diagnosed, making it np_local0.

originally, the tests for the range based for loops lived in the values part of the tests. As this is incorrect for the pointer-arrays I moved them into this file instead (for pointer arrays).
the tests here mostly verify that the variables are properly matched and filtered. The actual analysis has its own unit tests for these cases.

[JonasToth]

ping @njames93 :)

[njames93]

LGTM, just with one small testing nit

clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp

533

This not fixes line is pretty redundant, and unstable. If you really want to ensure no fix was generated, you can do a check-fixes with the original line contents. But as there is no warning generated here there shouldn't be a need to check for any fix behaviour, same goes below.

[JonasToth]

• remove unproductive check-fixes line

clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp

 //===--- ConstCorrectnessCheck.cpp - clang-tidy -----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ConstCorrectnessCheck.h"
 #include "../utils/FixItHintUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "llvm/Support/Casting.h"
 
 #include <iostream>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
if (IsNormalVariableInTemplate &&
       TemplateDiagnosticsCache.contains(Variable->getBeginLoc()))
     return;
 
   VariableCategory VC = VariableCategory::Value;
   if (Variable->getType()->isReferenceType())
     VC = VariableCategory::Reference;
   if (Variable->getType()->isPointerType())
     VC = VariableCategory::Pointer;
+  if (Variable->getType()->isArrayType()) {
+    if (const auto *ArrayT = dyn_cast<ArrayType>(Variable->getType())) {
+      if (ArrayT->getElementType()->isPointerType())
+        VC = VariableCategory::Pointer;
+    }
+  }
 
   // Each variable can only be in one category: Value, Pointer, Reference.
   // Analysis can be controlled for every category.
   if (VC == VariableCategory::Reference && !AnalyzeReferences)
     return;
 
   if (VC == VariableCategory::Reference &&
       Variable->getType()->getPointeeType()->isPointerType() &&

clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst

 .. title:: clang-tidy - misc-const-correctness
 
 misc-const-correctness
 ======================
 
 This check implements detection of local variables which could be declared as
-``const``, but are not. Declaring variables as ``const`` is required or recommended by many
+``const`` but are not. Declaring variables as ``const`` is required or recommended by many
 coding guidelines, such as:
 `CppCoreGuidelines ES.25 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on>`_
 and `AUTOSAR C++14 Rule A7-1-1 (6.7.1 Specifiers) <https://www.autosar.org/fileadmin/user_upload/standards/adaptive/17-03/AUTOSAR_RS_CPP14Guidelines.pdf>`_.
 
-Please note that this analysis is type-based only. Variables that are not modified
+Please note that this check's analysis is type-based only. Variables that are not modified
 but used to create a non-const handle that might escape the scope are not diagnosed
 as potential ``const``.
 
 .. code-block:: c++
 
   // Declare a variable, which is not ``const`` ...
   int i = 42;
   // but use it as read-only. This means that `i` can be declared ``const``.
-  int result = i * i;
+  int result = i * i;       // Before transformation
+  int const result = i * i; // After transformation
 
-The check can analyzes values, pointers and references but not (yet) pointees:
+The check can analyze values, pointers and references but not (yet) pointees:
 
 .. code-block:: c++
 
   // Normal values like built-ins or objects.
-  int potential_const_int = 42; // 'const int potential_const_int = 42' suggestion.
+  int potential_const_int = 42;       // Before transformation
+  int const potential_const_int = 42; // After transformation
   int copy_of_value = potential_const_int;
 
-  MyClass could_be_const; // 'const MyClass could_be_const' suggestion;
+  MyClass could_be_const;       // Before transformation
+  MyClass const could_be_const; // After transformation
   could_be_const.const_qualified_method();
 
   // References can be declared const as well.
-  int &reference_value = potential_const_int; // 'const int &reference_value' suggestion.
+  int &reference_value = potential_const_int;       // Before transformation
+  int const& reference_value = potential_const_int; // After transformation
   int another_copy = reference_value;
 
   // The similar semantics of pointers are not (yet) analyzed.
-  int *pointer_variable = &potential_const_int; // Not 'const int *pointer_variable' suggestion.
+  int *pointer_variable = &potential_const_int; // _NO_ 'const int *pointer_variable' suggestion.
   int last_copy = *pointer_variable;
 
 The automatic code transformation is only applied to variables that are declared in single
 declarations. You may want to prepare your code base with
-`readability-isolate-declaration <readability-isolate-declaration.html>`_ first.
+`readability-isolate-declaration <../readability/isolate-declaration.html>`_ first.
 
 Note that there is the check
-`cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines-avoid-non-const-global-variables.html>`_
+`cppcoreguidelines-avoid-non-const-global-variables <../cppcoreguidelines/avoid-non-const-global-variables.html>`_
 to enforce ``const`` correctness on all globals.
 
 Known Limitations
 -----------------
 
 The check will not analyze templated variables or variables that are instantiation dependent.
 Different instantiations can result in different ``const`` correctness properties and in general it
-is not possible to find all instantiations of a template. It might be used differently in an
-independent translation unit.
+is not possible to find all instantiations of a template. The template might be used differently in
+an independent translation unit.
 
-Pointees can not be analyzed for constness yet. The following code is shows this limitation.
+Pointees can not be analyzed for constness yet. The following code shows this limitation.
 
 .. code-block:: c++
 
   // Declare a variable that will not be modified.
   int constant_value = 42;
 
   // Declare a pointer to that variable, that does not modify either, but misses 'const'.
   // Could be 'const int *pointer_to_constant = &constant_value;'
   int *pointer_to_constant = &constant_value;
 
   // Usage:
   int result = 520 * 120 * (*pointer_to_constant);
 
 This limitation affects the capability to add ``const`` to methods which is not possible, too.
 
 Options
 -------
 
-.. option:: AnalyzeValues (default = 1)
+.. option:: AnalyzeValues (default = true)
 
   Enable or disable the analysis of ordinary value variables, like ``int i = 42;``
 
-.. option:: AnalyzeReferences (default = 1)
+  .. code-block:: c++
+
+    // Warning
+    int i = 42;
+    // No warning
+    int const i = 42;
+
+    // Warning
+    int a[] = {42, 42, 42};
+    // No warning
+    int const a[] = {42, 42, 42};
+
+.. option:: AnalyzeReferences (default = true)
 
   Enable or disable the analysis of reference variables, like ``int &ref = i;``
 
-.. option:: WarnPointersAsValues (default = 0)
+  .. code-block:: c++
+
+    int i = 42;
+    // Warning
+    int& ref = i;
+    // No warning
+    int const& ref = i;
+
+.. option:: WarnPointersAsValues (default = false)
 
   This option enables the suggestion for ``const`` of the pointer itself.
   Pointer values have two possibilities to be ``const``, the pointer
   and the value pointing to.
 
   .. code-block:: c++
 
-    const int value = 42;
-    const int * const pointer_variable = &value;
+    int value = 42;
 
-    // The following operations are forbidden for `pointer_variable`.
-    // *pointer_variable = 44;
-    // pointer_variable = nullptr;
+    // Warning
+    const int * pointer_variable = &value;
+    // No warning
+    const int *const pointer_variable = &value;
 
-.. option:: TransformValues (default = 1)
+.. option:: TransformValues (default = true)
 
-  Provides fixit-hints for value types that automatically adds ``const`` if its a single declaration.
+  Provides fixit-hints for value types that automatically add ``const`` if its a single declaration.
 
   .. code-block:: c++
 
-    // Emits a hint for 'value' to become 'const int value = 42;'.
+    // Before
     int value = 42;
+    // After
+    int const value = 42;
+
+    // Before
+    int a[] = {42, 42, 42};
+    // After
+    int const a[] = {42, 42, 42};
+
     // Result is modified later in its life-time. No diagnostic and fixit hint will be emitted.
     int result = value * 3;
     result -= 10;
 
-.. option:: TransformReferences (default = 1)
+.. option:: TransformReferences (default = true)
 
-  Provides fixit-hints for reference types that automatically adds ``const`` if its a single
+  Provides fixit-hints for reference types that automatically add ``const`` if its a single
   declaration.
 
   .. code-block:: c++
 
     // This variable could still be a constant. But because there is a non-const reference to
     // it, it can not be transformed (yet).
     int value = 42;
     // The reference 'ref_value' is not modified and can be made 'const int &ref_value = value;'
+    // Before
     int &ref_value = value;
+    // After
+    int const &ref_value = value;
 
     // Result is modified later in its life-time. No diagnostic and fixit hint will be emitted.
     int result = ref_value * 3;
     result -= 10;
 
-.. option:: TransformPointersAsValues (default = 0)
+.. option:: TransformPointersAsValues (default = false)
 
   Provides fixit-hints for pointers if their pointee is not changed. This does not analyze if the
   value-pointed-to is unchanged!
 
-  Requires 'WarnPointersAsValues' to be 1.
+  Requires 'WarnPointersAsValues' to be 'true'.
 
   .. code-block:: c++
 
     int value = 42;
-    // Emits a hint that 'ptr_value' may become 'int *const ptr_value = &value' because its pointee
-    // is not changed.
+
+    // Before
+    const int * pointer_variable = &value;
+    // After
+    const int *const pointer_variable = &value;
+
+    // Before
+    const int * a[] = {&value, &value};
+    // After
+    const int *const a[] = {&value, &value};
+
+    // Before
     int *ptr_value = &value;
+    // After
+    int *const ptr_value = &value;
 
-    int result = 100 * (*ptr_value);
-    // This modification of the pointee is still allowed and not analyzed/diagnosed.
+    int result = 100 * (*ptr_value); // Does not modify the pointer itself.
+    // This modification of the pointee is still allowed and not diagnosed.
     *ptr_value = 0;
 
     // The following pointer may not become a 'int *const'.
     int *changing_pointee = &value;
     changing_pointee = &result;
-

clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp

 // RUN: %check_clang_tidy %s misc-const-correctness %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: "misc-const-correctness.AnalyzeValues", value: true},\
 // RUN:   {key: "misc-const-correctness.WarnPointersAsValues", value: true},\
 // RUN:   {key: "misc-const-correctness.TransformPointersAsValues", value: true}]}' \
 // RUN: -- -fno-delayed-template-parsing
 
 void potential_const_pointer() {
   double np_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
   double *p_local0 = &np_local0[1];
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
   // CHECK-FIXES: double *const p_local0
+
+  using doublePtr = double*;
+  using doubleArray = double[15];
+  doubleArray np_local1;
+  doublePtr p_local1 = &np_local1[0];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'doublePtr' (aka 'double *') can be declared 'const'
+  // CHECK-FIXES: doublePtr const p_local1
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {&np_local0[0], &np_local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[2]
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }
+
+  int *p_local2[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local2[2]
+  for (const auto *con_ptr : p_local2) {
+  }
+
+}
+
+template <typename T>
+struct SmallVectorBase {
+  T data[4];
+  void push_back(const T &el) {}
+  int size() const { return 4; }
+  T *begin() { return data; }
+  const T *begin() const { return data; }
+  T *end() { return data + 4; }
+  const T *end() const { return data + 4; }
+};
+
+template <typename T>
+struct SmallVector : SmallVectorBase<T> {};
+
+template <class T>
+void EmitProtocolMethodList(T &&Methods) {
+  // Note: If the template is uninstantiated the analysis does not figure out,
+  // that p_local0 could be const. Not sure why, but probably bails because
+  // some expressions are type-dependent.
+  SmallVector<const int *> p_local0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector<const int *>' can be declared 'const'
+  // CHECK-FIXES: SmallVector<const int *> const p_local0
+  SmallVector<const int *> np_local0;
+  for (const auto *I : Methods) {
+    if (I == nullptr)
+      np_local0.push_back(I);
+  }
+  p_local0.size();
+}
+void instantiate() {
+  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[4]
+  EmitProtocolMethodList(p_local0);
 }

clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp

for (int value : p_local0) {
     // CHECK-FIXES: int const value
   }
 
   int p_local1[2] = {1, 2};
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int[2]' can be declared 'const'
   // CHECK-FIXES: int const p_local1[2]
   for (const int &const_ref : p_local1) {
   }
-
-  int *p_local2[2] = {&np_local0[0], &np_local0[1]};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local2[2]
-  for (const int *con_ptr : p_local2) {
-  }
 
-  int *p_local3[2] = {nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local3[2]
-  for (const auto *con_ptr : p_local3) {
-  }
+void arrays_of_pointers_are_ignored() {
+  int *np_local0[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: int * const np_local0[2]
 }
 
 inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
 
 struct Value {
 };
 void placement_new() {
   Value Mem;
void complex_usage() {
   (np_local0 == p_local0 ? np_local0 : (p_local0, np_local1))++;
 }
 
 void vlas() {
   int N = 1; // Can't make N 'const' because VLAs make everything awful
   sizeof(int[++N]);
 }
 
-template <typename T>
-struct SmallVectorBase {
-  T data[4];
-  void push_back(const T &el) {}
-  int size() const { return 4; }
-  T *begin() { return data; }
-  const T *begin() const { return data; }
-  T *end() { return data + 4; }
-  const T *end() const { return data + 4; }
-};
-
-template <typename T>
-struct SmallVector : SmallVectorBase<T> {};
-
-template <class T>
-void EmitProtocolMethodList(T &&Methods) {
-  // Note: If the template is uninstantiated the analysis does not figure out,
-  // that p_local0 could be const. Not sure why, but probably bails because
-  // some expressions are type-dependent.
-  SmallVector<const int *> p_local0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector<const int *>' can be declared 'const'
-  // CHECK-FIXES: SmallVector<const int *> const p_local0
-  SmallVector<const int *> np_local0;
-  for (const auto *I : Methods) {
-    if (I == nullptr)
-      np_local0.push_back(I);
-  }
-  p_local0.size();
-}
-void instantiate() {
-  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local0[4]
-  EmitProtocolMethodList(p_local0);
-}
 struct base {
   int member;
 };
 struct derived : base {};
 struct another_struct {
   derived member;
 };
 void another_struct_f() {