Skip to content

Add .. syntax to select a member function only #1101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 15, 2024

Conversation

hsutter
Copy link
Owner

@hsutter hsutter commented Jun 9, 2024

With this PR, x.f() is still UFCS, but x..f() will now find only a member function.

For now I like that the default is UFCS, but as we get more experience I'm open to changing the default so that . is member selection and .. is UFCS (which would be a breaking change, but a mechanical one).

Also: Remove the old version of get_declaration_of now that the new lookup seems stable.

With this commit, `x.f()` is still UFCS, but `x..f()` will now find only a member function.

For now I like that the default is UFCS, but as we get more experience I'm open to changing the default so that `.` is member selection and `..` is UFCS (which would be a breaking change, but a mechanical one).

Also: Remove the old version of `get_declaration_of` now that the new lookup seems stable.
@jarzec
Copy link
Contributor

jarzec commented Jun 10, 2024

@hsutter It looks like builds/regression tests on some platforms fail due to warnings that become errors. Fixing the problems should not be difficult. Would you like some help with that?
In any case, I would suggest merging #1095 first as it fixes some pre-existing regression test issues.

source/to_cpp1.h Outdated
@@ -272,6 +272,9 @@ class positional_printer
)
-> void
{
if (s.starts_with("..")) {
s == s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing "unused" warnings which lead to errors in the tests on some platforms. Is the if statement actually necessary?

Copy link
Contributor

@jarzec jarzec Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just checked that this is the reason for all failures among the multi-platform build tests.
The failures in the regression tests seem to be mainly due to necessary line updates in error messages. This should be quick to fix, but first the s == s needs to be sorted out, as this might further change line numbers.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I should remove that... that was so I could set a debug breakpoint on 276 that would fire only when .. was happening, to investigate why .. wasn't yet lowering to . correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so let me get the tests running. I will create a pull request into this branch.
Please give me a couple of minutes.

Copy link
Contributor

@jarzec jarzec Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it took a bit longer than a couple of minutes, but here is the PR into this one: #1103.

EDIT: Not sure if I made this clear. #1103 updates the test files to make CI all green again. You can either merge this one and #1103 right after or merge #1103 into this branch first and then the whole thing into the main branch.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This PR shouldn't affect any regression test output I think? The test generation should be identical, except only the one test I updated to add a .. test case which has expanded .cpp output but should not have any output changes because it doesn't print anything.

Re line# output: Usually the only kind of change that affects regression test line# output is when cpp2util.h changes cause the assertion line numbers to change for the handful of tests that a given older compiler doesn't fully support (typically GCC 10, sometimes Clang 12). So I don't think there should be line# regression test impact for this PR... I hope it's a pure extension!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, true, you are right.

Copy link
Contributor

@jarzec jarzec Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems some of the issues solved in #1103 are pre-existent (except the update of regression-tests/test-results/pure2-ufcs-member-access-and-chaining.cpp).
EDIT: Oh, even worse. Some of the issues fixed in #1103 are identical to those already merged, through. They would cause conflicts. I will remove those.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The tests on #1103 will be failing until you merge it into this branch. You can also merge this one with failures on pure2-ufcs-member-access-and-chaining.cpp and then #1103 to fix those.

source/sema.h Outdated
Comment on lines 485 to 510
////--------- START TEMPORARY REGRESSION TEST CODE FOR G_D_O OPTIMIZATION VERIFICATION ---------
//// Now do a regression test violation check
////
//if (
// flag_internal_debug
// && result_old != result
// )
//{
// std::cerr << "\n Internal compiler error - see cppfront-ice-data.out for debug information\n\n";

auto out = std::ofstream{"cppfront-ice-data.out"};
// auto out = std::ofstream{"cppfront-ice-data.out"};

out << "g_d_o arguments:\n";
out << " " << static_cast<void const*>(&t) << " -> " << t.as_string_view() << " @ " << t.position().to_string()
<< ", token order # " << t.get_global_token_order() << "\n";
out << " look_beyond_current_function: " << std::boolalpha << look_beyond_current_function << "\n";
out << " include_implicit_this: " << std::boolalpha << include_implicit_this << "\n";
// out << "g_d_o arguments:\n";
// out << " " << static_cast<void const*>(&t) << " -> " << t.as_string_view() << " @ " << t.position().to_string()
// << ", token order # " << t.get_global_token_order() << "\n";
// out << " look_beyond_current_function: " << std::boolalpha << look_beyond_current_function << "\n";
// out << " include_implicit_this: " << std::boolalpha << include_implicit_this << "\n";

out << "result_old: " << static_cast<void const*>(result_old) << "\n";
out << "result: " << static_cast<void const*>(result ) << "\n\n";
// out << "result_old: " << static_cast<void const*>(result_old) << "\n";
// out << "result: " << static_cast<void const*>(result ) << "\n\n";

debug_print(out);
// debug_print(out);

exit(EXIT_FAILURE);
}
//--------- END TEMPORARY REGRESSION TEST CODE FOR G_D_O OPTIMIZATION VERIFICATION -----------
// exit(EXIT_FAILURE);
//}
////--------- END TEMPORARY REGRESSION TEST CODE FOR G_D_O OPTIMIZATION VERIFICATION -----------
Copy link
Contributor

@jarzec jarzec Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was commented out for testing and can be removed now?
EDIT: If you do remove this code please don't merge #1103 yet. I will update it. This removal will likely require the error report lines in tests files to be updated again.

Suggested change
////--------- START TEMPORARY REGRESSION TEST CODE FOR G_D_O OPTIMIZATION VERIFICATION ---------
//// Now do a regression test violation check
////
//if (
// flag_internal_debug
// && result_old != result
// )
//{
// std::cerr << "\n Internal compiler error - see cppfront-ice-data.out for debug information\n\n";
auto out = std::ofstream{"cppfront-ice-data.out"};
// auto out = std::ofstream{"cppfront-ice-data.out"};
out << "g_d_o arguments:\n";
out << " " << static_cast<void const*>(&t) << " -> " << t.as_string_view() << " @ " << t.position().to_string()
<< ", token order # " << t.get_global_token_order() << "\n";
out << " look_beyond_current_function: " << std::boolalpha << look_beyond_current_function << "\n";
out << " include_implicit_this: " << std::boolalpha << include_implicit_this << "\n";
// out << "g_d_o arguments:\n";
// out << " " << static_cast<void const*>(&t) << " -> " << t.as_string_view() << " @ " << t.position().to_string()
// << ", token order # " << t.get_global_token_order() << "\n";
// out << " look_beyond_current_function: " << std::boolalpha << look_beyond_current_function << "\n";
// out << " include_implicit_this: " << std::boolalpha << include_implicit_this << "\n";
out << "result_old: " << static_cast<void const*>(result_old) << "\n";
out << "result: " << static_cast<void const*>(result ) << "\n\n";
// out << "result_old: " << static_cast<void const*>(result_old) << "\n";
// out << "result: " << static_cast<void const*>(result ) << "\n\n";
debug_print(out);
// debug_print(out);
exit(EXIT_FAILURE);
}
//--------- END TEMPORARY REGRESSION TEST CODE FOR G_D_O OPTIMIZATION VERIFICATION -----------
// exit(EXIT_FAILURE);
//}
////--------- END TEMPORARY REGRESSION TEST CODE FOR G_D_O OPTIMIZATION VERIFICATION -----------

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Yes I just forgot to remove that part. It shouldn't affect regression tests though. 👍

@jcanizales
Copy link

but x..f() will now find only a member function.

Hi, what is the use case this is addressing, if you don' t mind me asking?

@gregmarr
Copy link
Contributor

but x..f() will now find only a member function.

Hi, what is the use case this is addressing, if you don' t mind me asking?

Quite a bit of discussion at #1004

@jarzec
Copy link
Contributor

jarzec commented Jun 14, 2024

Is the PR ready to be merged?

@hsutter hsutter merged commit 9648191 into main Jun 15, 2024
30 of 50 checks passed
@hsutter
Copy link
Owner Author

hsutter commented Jun 15, 2024

(deleted some comments with extraneous info)

@JarekGlobus wrote:

A relatively simple solution to running Linux on Windows without security concerns is using Docker containers. It is not difficult to set up such a solution. Docket is very popular so there is a lot of online material on using it.

I'll take a look. Thanks!

Repository owner deleted a comment from JarekGlobus Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants