[clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused

[Johannes Doerfert via cfe-commits]

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 at lists.llvm.org> wrote:
 >
 >> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert 
<johannes at 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
 >