Skip to content
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

Add is and as support for std::expected (v2) #971

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bluetarpmedia
Copy link
Contributor

@bluetarpmedia bluetarpmedia commented Feb 7, 2024

  • Update cpp2util.h to support is and as for std::expected
  • Add new pure2-expected-is-as regression test

This replaces the original PR #954; I couldn't successfully rebase it onto the updated changes in main so I gave up and created a new PR!

CC @filipsajdak

EDIT - The macOS build job is a spurious failure (not found for compiler: 'clang++'). I'm not sure why this happens but I've seen it a few times now. I don't have permission to retry the job in this repo, but in my own fork I just retry the job and it succeeds.

EDIT 2 - See PR #972 for the spurious macOS job failure

Copy link
Contributor

@JohelEGP JohelEGP left a comment

Choose a reason for hiding this comment

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

I think you may have accidentally moved the built-in as code section.

@filipsajdak
Copy link
Contributor

@bluetarpmedia i will take a look after a decision on how to implement is and as functions. #701 (comment)

@bluetarpmedia
Copy link
Contributor Author

I think you may have accidentally moved the built-in as code section.

Thanks, restored now!

@hsutter
Copy link
Owner

hsutter commented Sep 25, 2024

Sorry for the lag one this. I think this in in pri-5, at least in part because I'm not sure if this PR has fallen behind the other is/as cleanup that was merged this summer (my fault, sorry).

Do you want to refresh this PR and it might move to pri-3?

@hsutter
Copy link
Owner

hsutter commented Oct 31, 2024

Hi! Do you want to pursue this now?

Also as described in #1322, this didn't make release 0.8, so I will need a new one-time CLA that will cover all your future contributions. I've emailed you the new CLA, and one it's completed I can look at PRs.

@bluetarpmedia bluetarpmedia marked this pull request as draft November 1, 2024 02:01
@bluetarpmedia
Copy link
Contributor Author

I've made a change to the CI run-tests.sh script to allow a compiler configuration to optionally exclude test files from running in that config.

This is because there are two macOS configurations which share the same expected results directory (to avoid duplication), since up to now their results have been identical.

However, I've added a new pure2-expected-is-as test which fails to compile on both compiler configs (which is correct, because they don't support std::expected), but they produce slightly different error diagnostics because of the different location of their standard libraries:

/Applications/Xcode_14.3.1.app/...etc.../MacOSX.sdk/usr/include/c++/v1/math.h:895:1: note: 'exp' declared here

and

/Library/Developer/...etc.../MacOSX13.sdk/usr/include/c++/v1/math.h:895:1: note: 'exp' declared here

One option would be to stop sharing the expected results directory, but that seemed wasteful since it's only 1 test file which produces a different result, and this test doesn't even compile (as expected). So it didn't seem necessary to me to verify the diagnostic since other compiler configs do that already.

However, please let me know if you prefer the duplication or a different solution instead of using this new filtering exclusion method.

FYI @jarzec

@bluetarpmedia
Copy link
Contributor Author

There are 2 ubuntu jobs failing but I believe this is unrelated to this PR's changes.

I've seen this happen before with #1198

Here's the diff:

-mixed-bounds-safety-with-assert.cpp2(11) void print_subrange(const auto:129&, cpp2::impl::in<int>, cpp2::impl::in<int>) [with auto:129 = std::vector<int>; cpp2::impl::in<int> = const int]: Bounds safety violation
+mixed-bounds-safety-with-assert.cpp2(11) void print_subrange(const auto:130&, cpp2::impl::in<int>, cpp2::impl::in<int>) [with auto:130 = std::vector<int>; cpp2::impl::in<int> = const int]: Bounds safety violation

GCC generates some kind of counter for the auto parameters.

@bluetarpmedia bluetarpmedia marked this pull request as ready for review November 2, 2024 01:09
@hsutter
Copy link
Owner

hsutter commented Nov 3, 2024

Thanks!

I've checked self-build and the first part of regression tests (building the .cpp2 tests to .cpp) and so far so good. But there seem to be weirdnesses when I get this branch, including that the compiler-specific regression tests folders all seem to be empty. (I worked around that it doesn't have all the venv/ files checked in for docs earlier this year).

I'm thinking that it would be best to rebase this -- could you do that please?

@hsutter hsutter added the question - further information requested Further information is requested label Nov 3, 2024
…es that it doesn't want to run

There are 2 macOS test configs which share the same expected results directory (to avoid duplication). This works great except for the new `pure2-expected-is-as` test. This new test code fails to compile (as expected) on both compilers, but produces a slightly different error diagnostic because the path is different.

E.g.

/Applications/Xcode_14.3.1.app/...etc.../math.h
and
/Library/Developer/CommandLineTools/...etc.../math.h

One option would be to stop sharing the expected results for both of these compilers, but that seems wasteful since it's just one test which fails to compile. So instead the `run-tests` script has a new way to exclude a test from running.
@bluetarpmedia
Copy link
Contributor Author

I'm thinking that it would be best to rebase this -- could you do that please?

Sure thing, I've rebased it now.

Not sure what was going on with those missing files you mentioned. If you still have problems maybe try deleting your local is-as-expected-v2 branch and then get a fresh checkout of it?

@jarzec
Copy link
Contributor

jarzec commented Nov 3, 2024

I've made a change to the CI run-tests.sh script to allow a compiler configuration to optionally exclude test files from running in that config.

This is because there are two macOS configurations which share the same expected results directory (to avoid duplication), since up to now their results have been identical.

However, I've added a new pure2-expected-is-as test which fails to compile on both compiler configs (which is correct, because they don't support std::expected), but they produce slightly different error diagnostics because of the different location of their standard libraries:

/Applications/Xcode_14.3.1.app/...etc.../MacOSX.sdk/usr/include/c++/v1/math.h:895:1: note: 'exp' declared here

and

/Library/Developer/...etc.../MacOSX13.sdk/usr/include/c++/v1/math.h:895:1: note: 'exp' declared here

One option would be to stop sharing the expected results directory, but that seemed wasteful since it's only 1 test file which produces a different result, and this test doesn't even compile (as expected). So it didn't seem necessary to me to verify the diagnostic since other compiler configs do that already.

However, please let me know if you prefer the duplication or a different solution instead of using this new filtering exclusion method.

FYI @jarzec

In fact there is a way to ignore diff chunks with given patterns - git has an option for that and the scripts exposes that. This was used in the past for similar issues with MSVC.
On the other hand the Clang v15 used so far on MacOs 13 comes from Homebrew. I've been thinking for a while that given that the results from version 14 are identical to those from 15 the 15 has little added value and can be dropped altogether.
I've had a look and GitHub now provides MasOs 15 runners where Clang 16 is used by XCode. I seems to make more sense now to drop the v15 and add the "official" Apple v16 instead.
I will try to have a look at al that.

@jarzec
Copy link
Contributor

jarzec commented Nov 3, 2024

There are 2 ubuntu jobs failing but I believe this is unrelated to this PR's changes.

I've seen this happen before with #1198

Here's the diff:

-mixed-bounds-safety-with-assert.cpp2(11) void print_subrange(const auto:129&, cpp2::impl::in<int>, cpp2::impl::in<int>) [with auto:129 = std::vector<int>; cpp2::impl::in<int> = const int]: Bounds safety violation
+mixed-bounds-safety-with-assert.cpp2(11) void print_subrange(const auto:130&, cpp2::impl::in<int>, cpp2::impl::in<int>) [with auto:130 = std::vector<int>; cpp2::impl::in<int> = const int]: Bounds safety violation

GCC generates some kind of counter for the auto parameters.

Given that you modified a header file all the test include I cam not surprised some internal numbering changed. I am no expert in in the internals of GCC, but I would say it is safe to accept those changes if the numbering is stable between runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants