-
Notifications
You must be signed in to change notification settings - Fork 6
Syncup with wlav/cppyy #155
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
Conversation
test/test_regression.py
Outdated
| def test45_typedef_resolution(self): | ||
| """Typedefs starting with 'c'""" | ||
|
|
||
| import cppyy | ||
|
|
||
| cppyy.cppdef("""\ | ||
| typedef const int my_custom_type_t; | ||
| typedef const int cmy_custom_type_t; | ||
| """) | ||
|
|
||
| assert cppyy.gbl.CppyyLegacy.TClassEdit.ResolveTypedef("my_custom_type_t") == "const int" | ||
| assert cppyy.gbl.CppyyLegacy.TClassEdit.ResolveTypedef("cmy_custom_type_t") == "const int" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test does not make sense with our fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to rework it and fix it upstream, too...
test/test_fragile.py
Outdated
| import cppyy | ||
|
|
||
| cppyy.cppdef("""\ | ||
| #if __has_include(<span>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #if __has_include(<span>) | |
| #if __cplusplus >= 202002 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this instead of the has_include?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at https://godbolt.org/z/6E8v4vooo with and without the -std=c++20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Can you make sure this is updated upstream too?
|
Hi @aaronj0, I guess you forgot to sync the |
…ena volatility, e.g. by the default compiler) from wlav@777f55e
This part of the test was performed incorrectly, the for loop uses j, and the value of i fixes the comparison to `short` only and ignores the other types; This caused the test to pass by chance. This has been rewritten with an outer loop that goes over the different type names (name) and j that iterates over the array indices. The values of `alpha` were also incorrect and did not reflect the multiplier used in the test dictionary. Another point to note is in the case of signed char, the pointer `m_schar_array2` is implicitly converted to `std::basic_string` while the fixed size array `m_schar_array` is not. Using the getter `get_schar_array2` however works here and we only compare using the instance retrieved this way and skip comparing `c.m_schar_array2` for this reason.
… us the same template proxy to f()
While this check consistently passes locally, sporadically failures are visibble on certain runners and is sensitive to the execution environment, especially CI machines under high load.
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! ;)
This PR brings our cppyy frontend up to date with wlav/cppyy:
Introduces all new tests (up to 540)
Adds new functions to the frontend (macro, as_memoryview)
Updates dictionaries for the same
Requires updating test tags based on CI results