[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 ↗

(On Diff #450359)

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 <iostream>
-
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 namespace {
 // FIXME: This matcher exists in some other code-review as well.
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/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};
+  
+  using intPtr = int*;
+  intPtr np_local1[2] = {nullptr, nullptr};
 }
 
 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() {