llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.24k stars 11.66k forks source link

[clang-tidy] readability-non-const-parameter: false positive in templated variable #60163

Open sebwolf-de opened 1 year ago

sebwolf-de commented 1 year ago

I find a false positive with clang-tidy's readability-non-const-parameter, if the access to the function parameter is hidden behind a template.

To reproduce:

#include <iostream>

constexpr size_t n = 20;

struct ForLoopRange {
  static constexpr size_t start{0};
  static constexpr size_t end{n};
  static constexpr size_t step{1};
};

template <typename Range>
inline void fillIfThreshold(double toFill[n],
                            const double inQuestion[n],
                            double fillValue) {
  constexpr double threshold = 0.001;
  for (auto index = Range::start; index < Range::end; index += Range::step) {
    if (inQuestion[index] > threshold) {
      toFill[index] = fillValue;
    }
  }
}

int main() {
  double toFill[n];
  double inQuestion[n];
  for (size_t i = 0; i < n; i++) {
    toFill[i] = 0;
    inQuestion[i] = 0.0001 * i;
  }
  const double fillValue = 12.0;
  fillIfThreshold<ForLoopRange>(toFill, inQuestion, fillValue);
  for (size_t i = 0; i < n; i++) {
    std::cout << toFill[i] << std::endl;
  }
}

If I run clang-tidy test.cpp, I get:

error: pointer parameter 'toFill' can be pointer to const [readability-non-const-parameter,-warnings-as-errors]
inline void fillIfThreshold(double toFill[n],
                                   ^
                            const

If I replace the for loop above with the version below, clang-tidy won't throw a false positive. for (size_t index = 0; index < n; index++) I use clang-tidy from the Fedora repositories:

> clang-tidy --version
LLVM (http://llvm.org/):
  LLVM version 15.0.7
  Optimized build.
  Default target: x86_64-redhat-linux-gnu
  Host CPU: skylake
llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-tidy

llvmbot commented 1 year ago

@llvm/issue-subscribers-bug

PiotrZSL commented 1 year ago

Found issue, bug is inside ArraySubscriptExpr class. Here we got:

as Base: DeclRefExpr 0x55905c7cdd48 'typename Range::Size' lvalue Var 0x55905c7cdab0 'index' 'typename Range::Size'
as Index: DeclRefExpr 0x55905c7cdd28 'double *':'double *' lvalue ParmVar 0x55905c7cd6a0 'toFill' 'double *':'double *'

clang/include/clang/AST/Expr.h:

bool lhsIsBase() const { return getRHS()->getType()->isIntegerType(); }

Because unknown type is not integer (template not resolved yet), then its used as Base. Proper way would be to also check if LHS is a pointer., and in worst case assume that LHS is base.

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-tidy

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-frontend