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.

Sign up for GitHub

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

Jump to bottom

Type system implementation #1: Added initial implementation for a new type system using redundancies. #9686

Merged
merged 9 commits into from
Aug 9, 2024
Merged

Type system implementation #1: Added initial implementation for a new type system using redundancies. #9686

merged 9 commits into from
Aug 9, 2024

Conversation

kc611
Copy link
Contributor

@kc611 kc611 commented Aug 5, 2024

This PR is a continuation of #9662, and adresses the review comments made in the same.

@kc611 kc611 added the skip_release_notes Skip towncrier requirement label Aug 5, 2024
@kc611 kc611 marked this pull request as draft August 5, 2024 12:50
@kc611 kc611 marked this pull request as ready for review August 6, 2024 14:29
@kc611 kc611 requested a review from gmarkall as a code owner August 6, 2024 14:29
@sklam sklam mentioned this pull request Aug 6, 2024
Merged
sklam
sklam reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@sklam sklam left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Suggestions for previous reviews that do not have related changes in this PR:

numba/core/datamodel/new_models.py Outdated Show resolved Hide resolved
numba/core/datamodel/new_models.py Outdated Show resolved Hide resolved
numba/core/datamodel/new_models.py Outdated Show resolved Hide resolved
numba/core/datamodel/new_models.py Outdated Show resolved Hide resolved
@kc611
Copy link
Contributor Author

kc611 commented Aug 7, 2024

RE: #9662 (comment)
Lack of boxing error handling will make it very difficult to debug problems.

Could you point to one such example of error handling in boxing, so that the intention here is more clear ?

@sklam
Copy link
Member

sklam commented Aug 8, 2024 •
edited
Loading

RE: #9686 (comment)

Key notes from offline discussion.

  • example

    numba/numba/core/old_boxing.py

    Line 609 in afb3d16

    with c.builder.if_else(cgutils.is_not_null(c.builder, obj)) as (has_parent, otherwise):
  • check NULL return value from python api and prevent segfault due to dereference NULL pointer

sklam
sklam reviewed Aug 8, 2024
View reviewed changes
Copy link
Member

@sklam sklam left a comment •
edited by kc611
Loading

Choose a reason for hiding this comment

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

I have gone through all the comments in 9662.

Missing review change:

numba/cpython/new_hashing.py Outdated Show resolved Hide resolved
numba/core/typing/new_builtins.py Outdated Show resolved Hide resolved
numba/core/typing/new_builtins.py Outdated Show resolved Hide resolved
@sklam sklam added 4 - Waiting on author Waiting for author to respond to review Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm and removed 3 - Ready for Review labels Aug 8, 2024
@sklam
Copy link
Member

sklam commented Aug 8, 2024

BFID: type_system_split_yaml_8

@sklam
Copy link
Member

sklam commented Aug 8, 2024

type_system_split_yaml_8 passed

@sklam sklam removed the 4 - Waiting on author Waiting for author to respond to review label Aug 9, 2024
@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels Aug 9, 2024
@sklam sklam added this to the 0.61.0-rc1 milestone Aug 9, 2024
@sklam sklam merged commit e980eba into numba:main Aug 9, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sklam sklam sklam approved these changes

@gmarkall gmarkall Awaiting requested review from gmarkall gmarkall is a code owner

Assignees
No one assigned
Labels
5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed skip_release_notes Skip towncrier requirement
Projects
None yet
Milestone
0.61.0-rc1
Development

Successfully merging this pull request may close these issues.
2 participants
@kc611 @sklam