Endre =?utf-8?q?Fülöp?= <[email protected]>, Endre =?utf-8?q?Fülöp?= <[email protected]>, Endre =?utf-8?q?Fülöp?= <[email protected]> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/[email protected]>
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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
