Page MenuHomePhabricator

Generalize clang-tidy modernize-pass-by-value
Needs ReviewPublic

Authored by mbid on Dec 1 2022, 8:34 AM.



Previously, the clang-tidy modernize-pass-by-value check would only fire
for arguments of constructors that are used exactly once and in a member
or base class initializers. This commit generalizes the check to general
functions and usages in the body of the function.

We now consider the control-flow graph of the function. If every
control-flow path must transition through one final usage of a given
parameter, we fire the diagnostic. We additionally require that the
variable is not referenced or pointed to by a local variable in the
function. There are still some false-positives, however, because we do
not try to figure out whether a pointer to the local variable escapes
through other means, e.g. here:

struct HasStrPtr {
  const std::string* str;

void set_ptr(HasStrPtr& has_ptr, const std::string& str) {
  has_ptr.str = &str;

// Take argument by value.
void foo(std::string);

void foo(const std::string& str) {
  HasStrPtr obj;
  set_ptr(obj, str);

The example looks at first sight like we can std::move in the call to
foo, but this would change the value pointed to by the pointer in
HasStrPtr, so that the semantics of the program change.

Diff Detail

Event Timeline

mbid created this revision.Dec 1 2022, 8:34 AM
Herald added a project: Restricted Project. · View Herald Transcript
mbid added a comment.Dec 1 2022, 8:38 AM

Hi Nathan,

Could you have a look at whether something like this looks reasonable? The docs of modernize-pass-by-value currently say that a patch that generalizes this check is welcome. I'd polish the docs and add some more test cases before merge if this looks reasonable overall.

  • Martin
mbid published this revision for review.Dec 1 2022, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 8:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ccotter added a subscriber: ccotter.Feb 5 2023, 8:40 PM
ccotter added inline comments.

I think you need to add clangAnalysis (for Analysis/CFG.h) to the link libraries in modernize/CMakeLists.txt. looks to be similar in part, as it applies a FixIt to a any last use of an object if the use a copy, including parameters and local objects.