[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

In D90042#2368219 <https://reviews.llvm.org/D90042#2368219>, @aaron.ballman wrote:

> In D90042#2360042 <https://reviews.llvm.org/D90042#2360042>, @flx wrote:
>
>> In D90042#2357078 <https://reviews.llvm.org/D90042#2357078>, @aaron.ballman wrote:
>>
>>> In D90042#2356265 <https://reviews.llvm.org/D90042#2356265>, @flx wrote:
>>>
>>>> In D90042#2356180 <https://reviews.llvm.org/D90042#2356180>, @aaron.ballman wrote:
>>>>
>>>>> In D90042#2350035 <https://reviews.llvm.org/D90042#2350035>, @flx wrote:
>>>>>
>>>>>> I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.
>>>>>
>>>>> Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?
>>>>
>>>> I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:
>>>>
>>>>   template <typename T>
>>>>   void foo(T&& t) {
>>>>     std::forward<T>(t).modify();
>>>>   }
>>>>
>>>> would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?
>>>
>>> Perhaps silly question, but are you instantiating `foo()`?
>>
>> I think I added a full implementation of foo now, reverted the change, but am still not getting the negative case to fail. Can you spot an issue with the code?
>
> I can't, but to be honest, I'm not certain I understand how that false positive could happen in the first place. That's why I was hoping to see the original case -- one thing you could try is with the original code, pass `-E` to preprocess to a file, and then try reducing the test case from that output (either by hand or by using a tool like creduce), or did you already give that a shot?

Thanks for the suggestion, I had never hear of creduce! After a bit of trial an error I seem to have found a more minimal example:

  namespace std {                                                                                                              
  template <typename> class function;                                                                                          
  template <typename a, typename... b> class function<a(b...)> {                                                               
  public:                                                                                                                      
    void operator()(b...);                                                                                                     
  };                                                                                                                           
  } // namespace std                                                                                                           
  struct c {                                                                                                                   
    c();                                                                                                                       
    c(const c &);                                                                                                              
  };                                                                                                                           
  std::function<void(c &)> f;                                                                                                  
  void d() {                                                                                                                   
    c Orig;                                                                                                                    
    c Copy = Orig;                                                                                                             
    f(Copy);                                                                                                                   
  }  

To be frank I can't spot a meaningful difference to the std::function copy we already have.

Here's also the test script I used for posterity:

  #!/bin/bash                                                                                                                  
                                                                                                                               
  trap 'exit 1' ERR                                                                                                            
  out=$(tempfile)                                                                                                              
  clang-tidy  --checks=-*,performance-unnecessary-copy-initialization --extra-arg=-std=c++17 full.cc > $out                                     
  grep "warning: local copy.*Copy" $out                                                                                        
  grep "f(Copy)" full.cc                                                                                                       
  grep "std::function" full.cc 

I'll update the test case with this next.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90042