cppfront icon indicating copy to clipboard operation
cppfront copied to clipboard

[FIX] multi return values handling

Open filipsajdak opened this issue 3 years ago • 1 comments
trafficstars

Closes https://github.com/hsutter/cppfront/issues/112

The current implementation fails to handle cases when there is a lambda in the function where multi-return values are used. The issue is with the get_declaration_of() function that tries not to go beyond the current function but is misled by a lambda function

//  Don't look beyond the current function
assert(decl.declaration);
if (decl.declaration->type.index() == declaration_node::function) {
    return nullptr;
}

It makes it impossible to compile below code:

fun: () -> (ri : int) = {
    ri = 0;

    pred := :(e:_) -> bool = { return e == 1; };

    ri = 42;

    return;
}

as the second call to ri variable is not getting a proper declaration from the get_declaration_of() function and it makes that cppfront generates:

#line 1 "tests/get_declarations_return_vals.cpp2"
[[nodiscard]] auto fun() -> fun__ret{
        cpp2::deferred_init<int> ri;
#line 2 "tests/get_declarations_return_vals.cpp2"
    ri.construct(0);

    auto pred { [](auto const& e) -> bool{return e == 1; } }; 

    ri = 42; // <--- lack of call to .value() method

    return  { std::move(ri.value()) }; 
}

This fix makes cppfront compile the above code to:

#line 1 "tests/get_declarations_return_vals.cpp2"
[[nodiscard]] auto fun() -> fun__ret{
        cpp2::deferred_init<int> ri;
#line 2 "tests/get_declarations_return_vals.cpp2"
    ri.construct(0);

    auto pred { [](auto const& e) -> bool{return e == 1; } }; 

    ri.value() = 42;

    return  { std::move(ri.value()) }; 
}

Also, this change fixes other issues when the multi-return values are initialized in the declaration place. The below code:

fun: () -> (ri : int = 0) = {
    pred := :(e:_) -> bool = { return e == 1; };

    ri = 42;

    return;
}

now compiles to:

#line 1 "/tests/get_declarations_return_vals.cpp2"
[[nodiscard]] auto fun() -> fun__ret{
    int ri = 0;
#line 2 "tests/get_declarations_return_vals.cpp2"
    auto pred { [](auto const& e) -> bool{return e == 1; } }; 

    ri = 42;

    return  { std::move(ri) }; // previously: return  { std::move(ri.value()) };
}

filipsajdak avatar Nov 12 '22 22:11 filipsajdak

I have a thought about what will happen when lambda itself will return multiple values. I have checked the code:

fun: () -> (ri : int) = {
    ri = 0;

    pred := :(e:_) -> (f:bool) = { f = e == 1; return; };

    ri = 42;

    return;
}

And I have an error message from cppfront that this is not a supported feature, yet.

get_declarations_return_vals.cpp2(4,57): error: an unnamed function at expression scope currently cannot return multiple values (at ';')

Unnamed functions also do not support contracts.

an unnamed function at expression scope currently cannot have contracts

cppfront also checks if there is no identifier set for an unnamed function - that makes my solution a good way of deducing if the function is indeed an unnamed function.

filipsajdak avatar Nov 13 '22 11:11 filipsajdak

Looks good, thanks!

I introduced conflicts so I applied essentially the same change as a separate commit instead of asking you to rebase. Please let me know if you spot a mistake, but it passes regression and your two test cases above.

hsutter avatar Dec 03 '22 00:12 hsutter