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

[Felix Berger via Phabricator via cfe-commits]

flx added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:126
   if (OldVar == nullptr) {
+    // Only allow initialization of a const reference from a free function if it
+    // has no arguments or only default arguments. Otherwise it could return
----------------
flx wrote:
> aaron.ballman wrote:
> > 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?
> 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.
This is done now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87455/new/

https://reviews.llvm.org/D87455