CI Fix tests by jarzec · Pull Request #1250 · hsutter/cppfront · GitHub

Fix an issue in mixed-type-safety-1.cpp2 and pure2-type-safety-1.cpp with C++23.
- The std::print() function added in C++23 caused issues with ADL leading to ambiguities of the print function template defined in the test itself.
- The proposed solution explicitly specifies the global namespace version at call sites. This approach minimizes the necessary changes.
Work around what seems to be an issue in the GCC 10 C++ compiler with requires expressions.
- For some reason the code compiled without issues by other compiler leads to linking issues (missing symbols) with GCC 10.
- A workaround was to define a concept to replace the requires expression.
As a result a number of compilation phase issue message files could be removed.
- In particular the issue with GCC 10 leading to messages including random names of the object files causing linking issues was fixed.
- The randomness made it impossible to make the test pass as a compilation failure.
- In an attempt to fix this, the other problems were uncovered and fixed.

As mentioned in the description of the PR replacing the requires expression using this concept allows GCC 10 to compile the generated C++ code.

I didn't know that we could write concepts in CPP2 already. Thank you for showing that.

It was a bit of a stab in the dark, but it worked. In fact this overload is not used at all in this test. One of the first things I tried was simply removing it altogether, which also made GCC 10 happy.
Then I tried to come up with a way to rewrite it to be able to keep it.

Explicit specification of the global namespace fixes an ambiguity issue caused by std::print.

I was trying to figure out why the cpp1 compiler considers std::print in this call... it looks like that compiler on some platforms makes them globally accessible.

I also found it strange, esp. that Clang-18 (C++23) was failing only for mixed-type-safety-1.cpp2, while MSVC (latest) was failing only for pure2-type-safety-1.cpp...

I was trying to figure out why the cpp1 compiler considers std::print in this call... it looks like that compiler on some platforms makes them globally accessible.

cpp2

print( "1   is int? ", 1   is int );
print("\ns* is Shape?  ", s* is Shape  );

cpp1

print( "1   is int? ", cpp2::impl::is<int>(1));
print("\ns* is Shape?  ", cpp2::impl::is<Shape>(*cpp2::impl::assert_not_null(s)));

cpp2::impl::is<...>(...) in these two calls resolves to std::true_type, so std::print is picked up by ADL.

Ah... right. The argument is from std namespace. I forgot about this rule.

@gregmarr Thank you!

Yes, I though it was ADL, as I rwrote in the PR description, but I found it strange there was come inconsistency in how differently the recent compiles (MSVC, GCC 14 and Clang-18) behave for mixed-type-safety-1.cpp2 on current main.

Yes, the inconsistency is strange. Do all of those compilers have std::print available with the compiler options that we are using? Otherwise they must just have different disambiguation rules.

Thanks for figuring this out! This was on my list to look into and I'm glad to see it's been figured out, much appreciated.

Should there be a comment there about why the :: is needed, so someone doesn't try to remove it in the future?

Should the function just be renamed?

Can we somehow make the user-facing end result of that is be bool even when it's the compile-time std::true_type or std::false_type so that users don't end up with these same ADL issues? Would that defeat the purpose of using those types in the first place? If so, do the benefits of returning that type outweigh the cost?

In fact renaming crossed my mind, but I thought full qualification would have extra educational value.
I wile the idea of adding a comment.

Looks good!

Thanks!

What's the best way to resolve the two .output file conflicts? Just delete them and regenerate them post-merge?

Normally I'd use the web editor but the button is disabled, saying the conflicts are too complex for the web editor.

@hsutter I do it locally and push it with the "Force with Lease" option. I will check if I can help...

@jarzec, can you rebase on the main branch? It will be the simplest solution.

@hsutter I do it locally and push it with the "Force with Lease" option. I will check if I can help...

@jarzec, can you rebase on the main branch? It will be the simplest solution.

Will do later today

Rebased with conflicts resolved.

Thank you!

I have added a comment and I propose to leave the solution with full qualification as it often is helpful when problems with ADL arise (speaking from my own experience).
Don't get me wrong, ADL is an awesome tool and very useful too. However, esp. with template resolution rules it can give a bit of a mindf$%k.
E.g. consider this: https://godbolt.org/z/8bM3cs8rf

In the version I sent the locally defined function is picked up.
If you uncomment line 13 the std version is picked up.
If you additionally uncomment line 4, still the std version is picked up.
If you now comment line 13 again (with line 4 uncommented) lo and behold: ambiguity 😁.
If you now change the type of the first argument of the function defined locally to char const* it will get picked up.
...

This is neverending and the devil is in the details...

Urk, it says to resolve conflicts again -- sorry, I may have done that because I just pushed a commit. 😞 Sorry, I didn't think it would interfere with this.

I don't know why it disables the "Resolve conflicts" button claiming that the conflicts are too complex to resolve in the web editor, those three files are not that long or complex. Sigh, sorry about that. I tried pulling the branch and rerunning regression on my machine but that didn't hit these files.

If you can re-fix these in the next couple of days I'll hold off on other commits.

I will rebase this on #1256 that already makes all tests green.

Urk, it says to resolve conflicts again -- sorry, I may have done that because I just pushed a commit. 😞 Sorry, I didn't think it would interfere with this.

I don't know why it disables the "Resolve conflicts" button claiming that the conflicts are too complex to resolve in the web editor, those three files are not that long or complex. Sigh, sorry about that. I tried pulling the branch and rerunning regression on my machine but that didn't hit these files.

If you can re-fix these in the next couple of days I'll hold off on other commits.

It is fixed. In fact this PR is now based on the updates in #1256. You can merge #1256 first.

Thanks! I have merged #1256. For some reason this is still showing conflicts that cannot be resolved in the web editor:

Conflicting files
regression-tests/test-results/clang-18-c++23-libcpp/mixed-type-safety-1.cpp.output
regression-tests/test-results/msvc-2022-c++latest/pure2-type-safety-1.cpp.output

Now that the conflicts are limited to .output files, perhaps the simplest thing is for me to just go delete them from this PR, and re-run regression separately...

This is weird: Only one of those two files exists in the branch. I tried deleting that one and pushed that, but it seems to have had no effect on the merge conflict.

Any ideas?

This is weird: Only one of those two files exists in the branch. I tried deleting that one and pushed that, but it seems to have had no effect on the merge conflict.

Any ideas?

I will try to fix that soon (as in later today).

Ready to merge.

Thanks!

jarzec commented Aug 20, 2024

View reviewed changes

filipsajdak approved these changes Aug 20, 2024

View reviewed changes

This was referenced Aug 20, 2024

is()/as(): refactor of is() and as() for variant to new design (part 2 of n) #1203

Merged

Update for sanitizing regex results and matcher strings. #1224

Merged

jarzec force-pushed the ci-fix-tests branch from b2c92c3 to de2c399 Compare August 22, 2024 18:17

jarzec force-pushed the ci-fix-tests branch from de2c399 to 3e7fdae Compare August 22, 2024 20:00

jarzec mentioned this pull request Aug 23, 2024

CI Update tests after recent modifs #1256

Merged

jarzec force-pushed the ci-fix-tests branch from 3e7fdae to cce9711 Compare August 25, 2024 23:07

jarzec force-pushed the ci-fix-tests branch from cce9711 to ce467b1 Compare August 26, 2024 21:54

hsutter mentioned this pull request Aug 27, 2024

[QUESTION] how to express this code in CppFront (is it possible)? #1235

Closed

jarzec added 4 commits August 27, 2024 20:54

CI Fix a C++23 template specialization issue

1488d45

CI Work around requires support issue in GCC 10

87738e6

CI Add explanatory comments to type safety tests

fc5f757

CI Remove unused file

c7f33dd

jarzec force-pushed the ci-fix-tests branch from b0cf701 to c7f33dd Compare August 27, 2024 19:09

hsutter merged commit c599f41 into hsutter:main Aug 27, 2024

gregmarr mentioned this pull request Oct 10, 2024

Exclude MSVC workaround on clang and gcc #1306

Merged

-              print: <T : type> ( msg: std::string, x: T )
-                     requires !std::convertible_to<T, bool> =
+              non_bool: <T> concept = !std::convertible_to<T, bool>;

               {
-                  print( "1.1 is int? ", 1.1 is int );
-                  print( "1   is int? ", 1   is int );
+                  ::print( "1.1 is int? ", 1.1 is int );

CI Fix tests by jarzec · Pull Request #1250 · hsutter/cppfront · GitHub

CI Fix tests - #1250

CI Fix tests#1250hsutter merged 4 commits intohsutter:mainhsutter/cppfront:mainfrom jarzec:ci-fix-testsjarzec/cppfront:ci-fix-testsCopy head branch name to clipboard

Conversation

jarzec commented Aug 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipsajdak left a comment

Choose a reason for hiding this comment

Uh oh!

hsutter commented Aug 21, 2024

Uh oh!

hsutter commented Aug 21, 2024

Uh oh!

filipsajdak commented Aug 21, 2024

Uh oh!

jarzec commented Aug 22, 2024

Uh oh!

jarzec commented Aug 22, 2024

Uh oh!

filipsajdak commented Aug 22, 2024

Uh oh!

jarzec commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsutter commented Aug 26, 2024

Uh oh!

jarzec commented Aug 26, 2024

Uh oh!

jarzec commented Aug 26, 2024

Uh oh!

hsutter commented Aug 27, 2024

Uh oh!

hsutter commented Aug 27, 2024

Uh oh!

jarzec commented Aug 27, 2024

Uh oh!

jarzec commented Aug 27, 2024

Uh oh!

hsutter commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CI Fix tests#1250
hsutter merged 4 commits into
hsutter:mainfrom
jarzec:ci-fix-tests

jarzec commented Aug 22, 2024 •
edited
Loading