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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
