gamesh411 wrote: @unterumarmung Thanks for the detailed feedback and the insight about the importance of the context in which these optional-like objects are used.
> 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 behaviour could still be > useful behind an option, e.g. `Mode: Aggressive` / `WarnOnAllValueOr`, but > I’m worried it will be noisy as the default. Having the context be the deciding factor behind the heuristic choice seems now the better decision, and maybe check-option about the optional-like-thing being rval could just become an automatic heuristic (as highlighted by @nicovank regarding that option). > 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. I ruled out fixits from the beginning, for the same reason you mentioned: it is hard to come up with an automatic refactoring in the general case, but if we consider the reference usages, it seems more manageable. I'll first check whether I can implement something that is worth the extra complexity for fixits. > 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. Thanks, I was not aware of this. > 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 behaviour. Agree, we should avoid suggesting anything in this case. > 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. Yes, this seems like the way. https://github.com/llvm/llvm-project/pull/200166 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
