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

[David Blaikie via cfe-commits]

Reverted in 0d0b90105f92f6cd9cc7004d565834f4429183fb & I'll see what
happens with the buildbots.

On Sun, Mar 22, 2020 at 5:47 PM Johannes Doerfert <johannes at 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 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
>  >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200322/b73e5dcd/attachment.html>