4

CLion (2023.1.2) is running clang-tidy v17 on a project of mine. It's complaining about me passing an std::experimental::optional (from C++14) by value instead of by reference. That seems egregious to me... how can I increase the "object weight threshold" for eliciting clang-tidy complaints about copying?

The exact problem description:

Clang-Tidy: The parameter 'whatever' is copied for each invocation but only used as a const reference; consider making it a const reference

Note: Unlike in this question, I want to change clang-tidy's threshold, not whitelist a specific line of code or specific type.

einpoklum
  • 118,144
  • 57
  • 340
  • 684
  • I only have clang-tidy 16 available and it doesn't complain for me. Is this new in v17? Btw, `sizeof(std::optional)` is `8` on my machine. – Ted Lyngmo May 23 '23 at 21:14
  • @TedLyngmo: Maybe it's a C++14 thing. I'll limit the question. Also, the size makes sense, because it has to be more than 4 to accommodate `nullopt`, and a size of 5 or 6 would be weird and problematic performance-wise. – einpoklum May 23 '23 at 22:02
  • Yeah. Well, I do get it when using `std::experimental::optional` in **both** C++14 and C++20 mode, while using `std::optional` in C++20 does not give that complaint. – Ted Lyngmo May 23 '23 at 22:09
  • I'm also trying to understand the clang-tidy code. Can't find a way to turn it off via a weight limit - but it's not code that's easy to read. It says _"Do not propose fixes when: 1. the ParmVarDecl is in a macro, since we cannot place them correctly, 2. the function is virtual as it might break overrides, 3. the function is referenced outside of a call expression within the compilation unit as the signature change could introduce build errors, 4. the function is a primary template or an explicit template specialization"_ - but mentions no size threshold or similar. – Ted Lyngmo May 23 '23 at 22:13
  • The above comments are from [`clang-tidy/performance/UnnecessaryValueParamCheck.cpp`](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp) btw – Ted Lyngmo May 23 '23 at 22:18
  • @TedLyngmo: So, it seems like you're saying that there is no such limit, and I should perhaps file a bug. But - have you noticed what criterion they use for the warning? I'll also go read it myself tomorrow. – einpoklum May 23 '23 at 22:40
  • Yes, that's as far as I got myself before I had to head to bed myself :-) Nice that ecatmur found the reason and perhaps a bug report/enhancement request would be in order. – Ted Lyngmo May 24 '23 at 06:19

1 Answers1

3

The clang-tidy check "performance-unnecessary-value-param" is looking for argument types that satisfy the "isExpensiveToCopy" predicate, which is a pretty simple test for non-trivial types:

  return !Type.isTriviallyCopyableType(Context) &&
         !classHasTrivialCopyAndDestroy(Type) &&
         !hasDeletedCopyConstructor(Type) &&
         !Type->isObjCLifetimeType();

This is reasonable in your case, because the TS std::experimental::optional does not propagate trivial copyability from its parameter, whereas std::optional does:

void f(std::optional<int>) {} // does not warn, because:
static_assert(std::is_trivially_copyable_v<std::optional<int>>);

void f(std::experimental::optional<int>) {} // warns, because:
static_assert(not std::is_trivially_copyable_v<std::experimental::optional<int>>);

The test is somewhat naive; the optimizer is capable of eliding the copy constructor of std::experimental::optional<int>. But there is no "weight" threshold. Indeed, the check does not warn on std::array<int, 1024>:

void f(std::array<int, 1024>) {} // does not warn, because:
static_assert(std::is_trivially_copyable_v<std::array<int, 1024>>);

Demo.

ecatmur
  • 152,476
  • 27
  • 293
  • 366