Page MenuHomePhabricator

[clang-tidy] performance-unnecessary-copy-initialization: Restrict UnnecessaryCopyInitialization check to variables initialized from free functions without arguments
ClosedPublic

Authored by flx on Sep 10 2020, 7:56 AM.

Details

Summary

This restriction avoids cases where an alias is returned to an argument and which could lead to to a false positive change. Example:

struct S {
  S(const S&);
  void modify();
};

const S& alias(const S& a) {
  return a;
}

void f(S a) {
  const S c = alias(a);
  a.modify();
}

Taking a const reference of the return value of alias would not be safe since a is modified.

A more involved alternative approach would be to inspect each argument passed to these functions for its constness, but this can get complex quickly.

Diff Detail

Event Timeline

flx created this revision.Sep 10 2020, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 7:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
flx requested review of this revision.Sep 10 2020, 7:56 AM
lebedev.ri retitled this revision from Restrict UnnecessaryCopyInitialization check to variables initialized from methods/functions without arguments to [clang-tidy] performance-unnecessary-copy-initialization: Restrict UnnecessaryCopyInitialization check to variables initialized from methods/functions without arguments .Sep 10 2020, 8:00 AM
flx added a comment.Sep 10 2020, 10:40 AM

The motivating false positive was the free functions std::min. To not limit the usefulness of the check I'll only disable arguments for free functions for now. Updated change coming.

flx updated this revision to Diff 291030.Sep 10 2020, 10:56 AM
flx retitled this revision from [clang-tidy] performance-unnecessary-copy-initialization: Restrict UnnecessaryCopyInitialization check to variables initialized from methods/functions without arguments to [clang-tidy] performance-unnecessary-copy-initialization: Restrict UnnecessaryCopyInitialization check to variables initialized from free functions without arguments .
flx edited the summary of this revision. (Show Details)
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a project: Restricted Project.
aaron.ballman added inline comments.Sep 14 2020, 7:59 AM
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
126

I'm not certain I agree with this change, but I'm also not certain I disagree with it. There are more ways to alias than just default arguments (global variables, etc) and the presence of a default argument is not really an indication about its return value in this case. e.g.,

const ExpensiveToCopyType &freeFunctionWithDefaultArg(int i = 12);

(I don't see a reason why = 12 should impact whether use of this function diagnoses or not.)

Perhaps if this was tightened up a bit to care about the type of the default argument it might make sense, but even that gets tricky when you consider base classes, template types, etc. and doesn't cover all of the aliasing cases anyway. Is there an important use case for this behavior?

flx added inline comments.Sep 14 2020, 8:21 AM
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
126

This was mostly following the logic already applied to the copy constructor where the existing code also allowed default arguments. The idea is to not disallow functions with default arguments that are not overridden.

I'll switch to not allowing any arguments. This will just exclude a few more cases, but better to be safe, until we might be able to verify the constness of all function arguments as well.

flx updated this revision to Diff 291584.Sep 14 2020, 8:55 AM
flx edited the summary of this revision. (Show Details)
flx added inline comments.
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
126

This is done now.

aaron.ballman accepted this revision.Sep 14 2020, 9:05 AM

LGTM with some minor nits.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
122

There's no real need for this change now, so I'd roll back to the previous form (then I don't have to wonder what the 1 means at the call site).

145

Some spurious whitespace changes that can also be rolled back.

This revision is now accepted and ready to land.Sep 14 2020, 9:05 AM
flx updated this revision to Diff 291610.Sep 14 2020, 10:43 AM
flx marked an inline comment as done.
flx marked an inline comment as done.
flx added a comment.Sep 14 2020, 10:53 AM

Thank you for the review, Aaron!

I don't have have commit access. Would you mind submitting it?

aaron.ballman closed this revision.Sep 15 2020, 5:47 AM
In D87455#2271582, @flx wrote:

Thank you for the review, Aaron!

I don't have have commit access. Would you mind submitting it?

I've commit on your behalf in 98e07b5596c8692c43770bc4e21a2b19467e35f7, thank you for the patch!