On Sun, Mar 22, 2020 at 1:48 PM David Blaikie via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert <johan...@jdoerfert.de> > wrote: > >> Some buildbots, I think only Windows buildbots for some reason, crashed >> in this function. >> >> The reason, as described, is that an `llvm::function_ref` cannot be >> copied and then reused. It just happened to work on almost all >> configurations. >> > > That doesn't seem to be accurate, or if it is there's a lot of mistakes in > the codebase - basically every function_ref parameter I can see in LLVM is > passing by value, not by const ref. The implementation of > llvm::function_ref looks quite copyable so long as it doesn't outlive the > functor it is pointing to. > David is correct. llvm::function_ref, like std::reference_wrapper, is a trivially copyable type, and it's designed to be copied. Like string_view and reference_wrapper, function_ref is designed to be passed by value. Redundantly passing function_ref *again by reference* is a code smell. I've also checked the code here, and it looks like there are only two callers of `anyScoreOrCondition` — both in Sema/SemaOpenMP.cpp — and they are both fine. FWIW, I would recommend reverting Johannes' change and seeing if those Windows buildbots are still unhappy (and if so, why). Btw, one failure mode I'm aware of, but which I believe is NOT relevant in Johannes' case, is that `function_ref`'s converting constructor behaves differently from its copy constructor. int main() { auto lamb = [](){ return 42; }; auto sheep = [](){ return 43; }; llvm::function_ref<int()> one = lamb; llvm::function_ref<int()> twoA = one; // twoA refers to lamb llvm::function_ref<short()> twoB = one; // twoB refers to one which refers to lamb one = sheep; assert(twoA() == 42); // twoA refers to lamb assert(twoB() == 43); // twoB refers to one which refers to sheep } That is, if you have a function that takes a parameter of type function_ref<A>, and someone passes it an argument of type function_ref<B>, then inside the function your parameter will be referring to that argument itself instead of to its referent. However, in Johannes' particular case, we have no function_refs referring to other function_refs. We just make a lambda and take a function_ref to it, the end. So I'm pretty sure this pitfall is irrelevant. –Arthur
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits