FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

CI Update default arg tests by jarzec · Pull Request #1263 · hsutter/cppfront · GitHub

/ cppfront Public

CI Update default arg tests - #1263

Merged
hsutter merged 2 commits into
hsutter:mainfrom
jarzec:ci-update-def-arg-test
Sep 23, 2024
ConversationCommits (2)ChecksFiles changed
Merged

CI Update default arg tests#1263
hsutter merged 2 commits into
hsutter:mainfrom
jarzec:ci-update-def-arg-test

Conversation

jarzec commented Aug 27, 2024

Copy link
Copy Markdown
Contributor

As stated in the title.

jarzec commented Aug 27, 2024
View reviewed changes
Comment thread regression-tests/test-results/gcc-14-c++2b/pure2-default-arguments.cpp.execution Outdated
calling: int main(int, char**)
012
a newer compiler
an older compiler

jarzec Aug 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this intended? 14 is the most recent version of GCC released only a few months back.

gregmarr Aug 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this is a bug in

cppfront/include/cpp2util.h

Line 367 in 2e23597

return gcc_ver > gcc || clang_ver > clang || msvc_ver > msvc;
and they should be >= rather than >.

jarzec Aug 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It does smell like it. @hsutter do you confirm, should I fix it in this PR?

filipsajdak Aug 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This test does not test whether gcc-14 is modern but whether you can test its version at compile time.

jarzec Aug 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree, but it is still a bit strange to see a very recent compiler marked as old.
To me the name gcc_clang_msvc_min_versions suggest that if the actual version is equal to the value stored internally the version should be accepted.

jarzec commented Aug 28, 2024
View reviewed changes
Comment thread include/cpp2util.h
jarzec force-pushed the ci-update-def-arg-test branch from 354e011 to bb433a2 Compare August 28, 2024 20:06
jarzec commented Aug 28, 2024
View reviewed changes
Comment thread regression-tests/test-results/gcc-14-c++2b/pure2-range-operators.cpp.execution
false
false

Make sure views::take works:

jarzec Aug 28, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Now that GCC 14 is considered newer compiler it is allowed to do this as well.

jarzec mentioned this pull request Aug 28, 2024
Closed

jarzec commented Sep 4, 2024

Copy link
Copy Markdown
Contributor Author

@hsutter If you agree with the outcome of the discussions this test update is ready to go.
I will address other test failures on the main branch in a separate PR.

hsutter commented Sep 23, 2024

Copy link
Copy Markdown
Owner

Thanks!

hsutter merged commit b3e7ef1 into hsutter:main Sep 23, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
filipsajdak filipsajdak left review comments
gregmarr gregmarr left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL