<div dir="ltr">Reverted in 0d0b90105f92f6cd9cc7004d565834f4429183fb & I'll see what happens with the buildbots.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Mar 22, 2020 at 5:47 PM Johannes Doerfert <<a href="mailto:johannes@jdoerfert.de">johannes@jdoerfert.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Apologies for the confusion.<br>
<br>
I wrote the commit message after looking into this and I though the<br>
issue was related to the capture by copy in the inner llvm::any_of and<br>
the reuse in the outer. Looking back at the code I cannot say anymore<br>
how I got that impression.<br>
<br>
If you think the reference is problematic, I'm totally happy to remove<br>
it. If the windows bots (or any other ones) don't like it we need to<br>
investigate why. As mentioned, I had a problem recreating the problem<br>
locally before.<br>
<br>
Cheers,<br>
   Johannes<br>
<br>
<br>
On 3/22/20 1:37 PM, Arthur O'Dwyer wrote:<br>
 > On Sun, Mar 22, 2020 at 1:48 PM David Blaikie via cfe-commits <<br>
 > <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
 ><br>
 >> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert <br>
<<a href="mailto:johannes@jdoerfert.de" target="_blank">johannes@jdoerfert.de</a>><br>
 >> wrote:<br>
 >><br>
 >>> Some buildbots, I think only Windows buildbots for some reason, crashed<br>
 >>> in this function.<br>
 >>><br>
 >>> The reason, as described, is that an `llvm::function_ref` cannot be<br>
 >>> copied and then reused. It just happened to work on almost all<br>
 >>> configurations.<br>
 >>><br>
 >><br>
 >> That doesn't seem to be accurate, or if it is there's a lot of <br>
mistakes in<br>
 >> the codebase - basically every function_ref parameter I can see in <br>
LLVM is<br>
 >> passing by value, not by const ref. The implementation of<br>
 >> llvm::function_ref looks quite copyable so long as it doesn't <br>
outlive the<br>
 >> functor it is pointing to.<br>
 >><br>
 ><br>
 > David is correct. llvm::function_ref, like std::reference_wrapper, is a<br>
 > trivially copyable type, and it's designed to be copied.<br>
 > Like string_view and reference_wrapper, function_ref is designed to be<br>
 > passed by value. Redundantly passing function_ref *again by <br>
reference* is a<br>
 > code smell.<br>
 ><br>
 > I've also checked the code here, and it looks like there are only two<br>
 > callers of `anyScoreOrCondition` — both in Sema/SemaOpenMP.cpp — and they<br>
 > are both fine. FWIW, I would recommend reverting Johannes' change and<br>
 > seeing if those Windows buildbots are still unhappy (and if so, why).<br>
 ><br>
 ><br>
 > Btw, one failure mode I'm aware of, but which I believe is NOT <br>
relevant in<br>
 > Johannes' case, is that `function_ref`'s converting constructor behaves<br>
 > differently from its copy constructor.<br>
 ><br>
 > int main()<br>
 ><br>
 > {<br>
 ><br>
 >     auto lamb = [](){ return 42; };<br>
 ><br>
 >     auto sheep = [](){ return 43; };<br>
 ><br>
 >     llvm::function_ref<int()> one = lamb;<br>
 ><br>
 ><br>
 >     llvm::function_ref<int()> twoA = one;    // twoA refers to lamb<br>
 ><br>
 >     llvm::function_ref<short()> twoB = one;  // twoB refers to one which<br>
 > refers to lamb<br>
 ><br>
 ><br>
 >     one = sheep;<br>
 ><br>
 ><br>
 >     assert(twoA() == 42);  // twoA refers to lamb<br>
 ><br>
 >     assert(twoB() == 43);  // twoB refers to one which refers to sheep<br>
 ><br>
 > }<br>
 ><br>
 > That is, if you have a function that takes a parameter of type<br>
 > function_ref<A>, and someone passes it an argument of type <br>
function_ref<B>,<br>
 > then inside the function your parameter will be referring to that <br>
argument<br>
 > itself instead of to its referent.<br>
 > However, in Johannes' particular case, we have no function_refs referring<br>
 > to other function_refs. We just make a lambda and take a function_ref to<br>
 > it, the end. So I'm pretty sure this pitfall is irrelevant.<br>
 ><br>
 > –Arthur<br>
 ><br>
<br>
</blockquote></div>