[clang-tools-extra] [clang-tidy] Add performance-expensive-value-or check (PR #200166)

[Daniil Dudkin via cfe-commits]

Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>,
Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>,
Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/200166 at github.com>


https://github.com/unterumarmung commented:

I’m not a clang-tidy maintainer, so please treat this as design feedback rather than a strong objection.

I think this check may be too broad as a default `performance-*` warning. `value_or()` returning by value is not necessarily an avoidable copy. For example, these uses are explicitly asking for an owned `T`:

```cpp
std::string S = Opt.value_or("fallback");

consumeString(Opt.value_or("fallback")); // void consumeString(std::string);
```

A rewrite based on `operator*`/`value()` is not necessarily an improvement in those cases:

```cpp
std::string S = Opt.value_or("fallback");

// This is closer in shape, but it still produces an owned std::string. The
// suggested `operator*`/`value()` direction does not really avoid that final
// copy/construction when ownership is the desired result.
std::string S = Opt ? *Opt : std::string("fallback");
```

It is also questionable when the result is passed to an API that intentionally takes ownership or needs a mutable value:

```cpp
void consumeString(std::string);

consumeString(Opt.value_or("fallback"));

// A reference-based rewrite does not match what this call expresses.
const std::string Fallback = "fallback";
consumeString(Opt ? *Opt : Fallback);
```

In contrast, the check seems much more actionable in reference-friendly contexts:

```cpp
const std::string &S = Opt.value_or(Fallback);
consumeStringRef(Opt.value_or(Fallback)); // void consumeStringRef(const std::string &);
Opt.value_or(Fallback).size();
```

Those can naturally become:

```cpp
const std::string &S = Opt ? *Opt : Fallback;
consumeStringRef(Opt ? *Opt : Fallback);
(Opt ? *Opt : Fallback).size();
```

Could we make the default mode more conservative and only warn when the result is used in one of those reference-friendly contexts, for example binding to `const T &`, passing to a `const T &` parameter, or immediately calling a const member on the temporary? The broader behavior could still be useful behind an option, e.g. `Mode: Aggressive` / `WarnOnAllValueOr`, but I’m worried it will be noisy as the default.

A few related ergonomics points: the diagnostic should probably avoid directly recommending `operator*`/`value()` in all cases, because that is not always semantically equivalent. For example, if the fallback has side effects, a naive rewrite can change behavior:

```cpp
std::string makeFallback();

std::string S = Opt.value_or(makeFallback()); // makeFallback() is always called

// Not equivalent: makeFallback() is only called when Opt is empty.
const std::string &S = Opt ? *Opt : makeFallback();
```

So something like “`value_or` returns by value and may copy expensive type %0” would be safer as the general diagnostic, with the stronger suggestion only in contexts where a reference-preserving rewrite is actually appropriate.

It would also be good to reuse or align with the existing `clang-tidy/utils/TypeTraits.cpp:isExpensiveToCopy` helper, then layer the size threshold on top if large trivially-copyable objects are intentionally in scope.

Finally, optional-type matching could probably follow the existing optional-related checks more closely, including the `std`/`absl`/`boost` defaults and regex-based matching, so the configuration is consistent with the rest of clang-tidy. Right now a user has to spell out additional optional-like types exactly:

```yaml
CheckOptions:
  performance-expensive-value-or.OptionalTypes: "::std::optional;::absl::optional"
```

It would be more consistent with existing optional checks to default to the common optional-like types and allow regex-style entries, e.g. matching project-local optional wrappers without enumerating every qualified spelling:

```yaml
CheckOptions:
  performance-expensive-value-or.OptionalTypes: "::std::optional;::absl::optional;::boost::optional;::my_project::.*Optional"
```

That would also make the docs’ claim that the check supports `boost::optional`, `absl::optional`, and other optional-like types feel more natural out of the box.

https://github.com/llvm/llvm-project/pull/200166