Skip to content

Revert "Revert "Fix get_nested_resource_ptr to accept both str and bytes inputs""#1698

Open
rparolin wants to merge 2 commits intomainfrom
revert-1697-revert-1665-rparolin/get_nested_resource_fix
Open

Revert "Revert "Fix get_nested_resource_ptr to accept both str and bytes inputs""#1698
rparolin wants to merge 2 commits intomainfrom
revert-1697-revert-1665-rparolin/get_nested_resource_fix

Conversation

@rparolin
Copy link
Collaborator

Reverts #1697

@copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Feb 27, 2026

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@rparolin
Copy link
Collaborator Author

/ok to test

@github-actions
Copy link

@rparolin
Copy link
Collaborator Author

/ok to test

Comment on lines -126 to +134
obj_i_ptr = <char*>(obj_i_bytes)
obj_i_ptr = PyBytes_AsString(obj_i_bytes)
Copy link
Member

Choose a reason for hiding this comment

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

In general we should let Cython figure out the correct C API to use, and not intervene with manual C API calls unless we can prove

  1. Cython emits wrong code, or
  2. We can do it with an equally safe but more performant way

nvjitlink.destroy(handle)


@pytest.mark.parametrize("option", ARCHITECTURES)
Copy link
Member

Choose a reason for hiding this comment

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

no need to parametrize it, just one bytes instance is fine

Comment on lines +118 to +120
@pytest.mark.parametrize(
"options", [[], ["-opt=0"], ["-opt=3", "-g"], [b"-opt=0"], [b"-opt=3", b"-g"], ["-opt=3", b"-g"]]
)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines +140 to +142
@pytest.mark.parametrize(
"options", [[], ["-opt=0"], ["-opt=3", "-g"], [b"-opt=0"], [b"-opt=3", b"-g"], ["-opt=3", b"-g"]]
)
Copy link
Member

Choose a reason for hiding this comment

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

ditto; also, if this is already tested above, no need to test again

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants