Skip to content

Commit

Permalink
Fix generation when operator= nests a lambda, and work around Clang…
Browse files Browse the repository at this point in the history 
… and MSVC bug, addresses hsutter#281

Also fix the logic in the `is_constructor_*` and `is_assignment_*` functions
  • Loading branch information
@hsutter @zaucy
hsutter authored and zaucy committed Dec 5, 2023
1 parent 7d47c53 commit 978be9a
Show file tree
Hide file tree
Showing 3 changed files with 189 additions and 98 deletions.
26 changes: 21 additions & 5 deletions include/cpp2util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -630,8 +630,25 @@ class out {
#define CPP2_FORCE_INLINE_LAMBDA __attribute__((always_inline))
#endif


// === NOTE: (see also corresponding note in cppfront.cpp)
//
// This "&" capture default is to work around 'if constexpr' bugs in Clang and MSVC...
// If we don't emit a not-actually-used "[&]" here, then both compilers get tripped up
// if we attempt UFCS for a member function name... the reason is that the UFCS macro
// generates a lambda that does an 'if constexpr' to call the member function if
// available, but Clang and MSVC look into the NOT-taken 'if constexpr' branch (which
// they shouldn't) and see the nonmember function call syntax and think it's trying to
// use the member function with implicit 'this->' and choke... (see issue #281, and
// https://godbolt.org/z/M47zzMsoT for a distilled repro)
//
// The workaround that seems to be effective is to add TWO actually-unused "&" captures:
// - here, and
// - and also in the cppfront.cpp build_capture_lambda_intro_for() code
//

#define CPP2_UFCS(FUNCNAME,PARAM1,...) \
[](auto&& obj, auto&& ...params) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
[&](auto&& obj, auto&& ...params) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
if constexpr (requires{ std::forward<decltype(obj)>(obj).FUNCNAME(std::forward<decltype(params)>(params)...); }) { \
return std::forward<decltype(obj)>(obj).FUNCNAME(std::forward<decltype(params)>(params)...); \
} else { \
Expand All @@ -640,7 +657,7 @@ class out {
}(PARAM1, __VA_ARGS__)

#define CPP2_UFCS_0(FUNCNAME,PARAM1) \
[](auto&& obj) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
[&](auto&& obj) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
if constexpr (requires{ std::forward<decltype(obj)>(obj).FUNCNAME(); }) { \
return std::forward<decltype(obj)>(obj).FUNCNAME(); \
} else { \
Expand All @@ -651,7 +668,7 @@ class out {
#define CPP2_UFCS_REMPARENS(...) __VA_ARGS__

#define CPP2_UFCS_TEMPLATE(FUNCNAME,TEMPARGS,PARAM1,...) \
[](auto&& obj, auto&& ...params) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
[&](auto&& obj, auto&& ...params) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
if constexpr (requires{ std::forward<decltype(obj)>(obj).template FUNCNAME CPP2_UFCS_REMPARENS TEMPARGS (std::forward<decltype(params)>(params)...); }) { \
return std::forward<decltype(obj)>(obj).template FUNCNAME CPP2_UFCS_REMPARENS TEMPARGS (std::forward<decltype(params)>(params)...); \
} else { \
Expand All @@ -660,14 +677,13 @@ class out {
}(PARAM1, __VA_ARGS__)

#define CPP2_UFCS_TEMPLATE_0(FUNCNAME,TEMPARGS,PARAM1) \
[](auto&& obj) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
[&](auto&& obj) CPP2_FORCE_INLINE_LAMBDA -> decltype(auto) { \
if constexpr (requires{ std::forward<decltype(obj)>(obj).template FUNCNAME CPP2_UFCS_REMPARENS TEMPARGS (); }) { \
return std::forward<decltype(obj)>(obj).template FUNCNAME CPP2_UFCS_REMPARENS TEMPARGS (); \
} else { \
return FUNCNAME CPP2_UFCS_REMPARENS TEMPARGS (std::forward<decltype(obj)>(obj)); \
} \
}(PARAM1)
//--------------------------------------------------------------------


//-----------------------------------------------------------------------
Expand Down
81 changes: 57 additions & 24 deletions source/cppfront.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,11 +835,11 @@ class cppfront
bool violates_initialization_safety = false;
bool suppress_move_from_last_use = false;

bool generating_assignment_function = false;
bool generating_move_function = false;
bool emitting_that_function = false;
bool emitting_move_that_function = false;
std::vector<token const*> already_moved_that_members = {};
declaration_node const* generating_assignment_from = {};
declaration_node const* generating_move_from = {};
bool emitting_that_function = false;
bool emitting_move_that_function = false;
std::vector<token const*> already_moved_that_members = {};

struct arg_info {
passing_style pass = passing_style::in;
Expand All @@ -848,10 +848,18 @@ class cppfront
std::vector<arg_info> current_args = { {} };

struct function_info {
token const* pname = {};
declaration_node const* decl = {};
declaration_node::declared_that_funcs declared_that_functions = {};
function_prolog prolog = {};
std::vector<std::string> epilog = {};

function_info(
declaration_node const* decl_,
declaration_node::declared_that_funcs declared_that_functions_
)
: decl{decl_}
, declared_that_functions{declared_that_functions_}
{ }
};
std::vector<function_info> current_function_info = { {} };

Expand Down Expand Up @@ -1965,7 +1973,7 @@ class cppfront

// Return without expression, could be assignment operator
//
else if (generating_assignment_function)
else if (generating_assignment_from == current_function_info.back().decl)
{
printer.print_cpp2("*this", n.position());
}
Expand Down Expand Up @@ -2079,9 +2087,35 @@ class cppfront

// Then build the capture list, ignoring duplicated expressions
auto lambda_intro = std::string("[");
auto num_captures = 0;

// === NOTE: (see also corresponding note in cpp2util.h)
//
// This "&" capture default is to work around 'if constexpr' bugs in Clang and MSVC...
// If we don't emit a not-actually-used "[&]" here, then both compilers get tripped up
// if we attempt UFCS for a member function name... the reason is that the UFCS macro
// generates a lambda that does an 'if constexpr' to call the member function if
// available, but Clang and MSVC look into the NOT-taken 'if constexpr' branch (which
// they shouldn't) and see the nonmember function call syntax and think it's trying to
// use the member function with implicit 'this->' and choke... (see issue #281, and
// https://godbolt.org/z/M47zzMsoT for a distilled repro)
//
// The workaround that seems to be effective is to add TWO actually-unused "&" captures:
// - here, and
// - and also in the cpp2util.h UFCS code
//
if (
!current_function_info.empty()
&& current_function_info.back().decl->is_function_with_this()
)
{
lambda_intro += "&";
++num_captures;
}
// === END NOTE

printer.emit_to_string(&lambda_intro);

auto num = 0;
auto handled = std::vector<std::string>{};
for (auto& cap : captures.members)
{
Expand All @@ -2092,13 +2126,13 @@ class cppfront
handled.push_back(cap.str);

// And handle it
if (num != 0) { // not first
if (num_captures != 0) { // not first
lambda_intro += ", ";
}
cap.cap_sym = "_"+std::to_string(num);
cap.cap_sym = "_"+std::to_string(num_captures);
printer.print_cpp2(cap.cap_sym + " = " + cap.str, pos);
}
++num;
++num_captures;
}
printer.emit_to_string();
lambda_intro += "]";
Expand Down Expand Up @@ -3320,8 +3354,7 @@ class cppfront
{
if (
n.pass != passing_style::out
|| !current_function_info.back().pname
|| *current_function_info.back().pname != "operator="
|| !current_function_info.back().decl->has_name("operator=")
)
{
errors.emplace_back(
Expand Down Expand Up @@ -3725,7 +3758,7 @@ class cppfront
// Handle a special member function
if (
n.is_assignment()
|| generating_assignment_function
|| generating_assignment_from == n.my_decl
)
{
assert(
Expand Down Expand Up @@ -3914,7 +3947,7 @@ class cppfront
assert(func);

auto is_assignment =
generating_assignment_function
generating_assignment_from == &n
|| (*func->parameters)[0]->pass == passing_style::inout;

if (
Expand All @@ -3925,7 +3958,7 @@ class cppfront
emitting_that_function = true;
if (
(*func->parameters)[1]->pass == passing_style::move
|| generating_move_function
|| generating_move_from == &n
)
{
emitting_move_that_function = true;
Expand Down Expand Up @@ -4315,7 +4348,7 @@ class cppfront
assert(func);

current_function_info.emplace_back(
n.identifier ? n.name() : nullptr,
&n,
n.find_parent_declared_that_functions()
);
auto guard = finally([&]{ current_function_info.pop_back(); });
Expand Down Expand Up @@ -4381,7 +4414,7 @@ class cppfront
if (
func->is_constructor()
&& func->parameters->ssize() == 2
&& !generating_assignment_function
&& generating_assignment_from != &n
)
{
prefix += "explicit ";
Expand Down Expand Up @@ -4427,7 +4460,7 @@ class cppfront
// so recurse to emit related functions if the user didn't write them by hand
if (
func->parameters->ssize() == 2
&& !generating_assignment_function
&& generating_assignment_from != &n
)
{
assert(!current_function_info.empty());
Expand Down Expand Up @@ -4469,7 +4502,7 @@ class cppfront
need_to_generate_assignment = true;
}

if (!generating_move_function) {
if (generating_move_from != &n) {

// M) Generate (M)ove from copy,
// if the user didn't write the move function themselves
Expand Down Expand Up @@ -4721,9 +4754,9 @@ class cppfront

// Then reposition and do the recursive call
printer.reset_cpp2_line_to(n.position().lineno);
generating_assignment_function = true;
generating_assignment_from = &n;
emit( n, capture_intro );
generating_assignment_function = false;
generating_assignment_from = {};
}

// If this was a constructor and we want also want to emit
Expand All @@ -4737,9 +4770,9 @@ class cppfront

// Then reposition and do the recursive call
printer.reset_cpp2_line_to(n.position().lineno);
generating_move_function = true;
generating_move_from = &n;
emit( n, capture_intro );
generating_move_function = false;
generating_move_from = {};
}
}

Expand Down
Loading

0 comments on commit 978be9a

Please sign in to comment.