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

[Arthur O'Dwyer via cfe-commits]

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/794d4848/attachment-0001.html>