Reverted in 0d0b90105f92f6cd9cc7004d565834f4429183fb & I'll see what happens with the buildbots.
On Sun, Mar 22, 2020 at 5:47 PM Johannes Doerfert <johan...@jdoerfert.de> wrote: > > Apologies for the confusion. > > I wrote the commit message after looking into this and I though the > issue was related to the capture by copy in the inner llvm::any_of and > the reuse in the outer. Looking back at the code I cannot say anymore > how I got that impression. > > If you think the reference is problematic, I'm totally happy to remove > it. If the windows bots (or any other ones) don't like it we need to > investigate why. As mentioned, I had a problem recreating the problem > locally before. > > Cheers, > Johannes > > > On 3/22/20 1:37 PM, Arthur O'Dwyer wrote: > > 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