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

Introduce HPy_DelAttr(_s) and HPy_DelItem(_i/_s) #372

Merged
merged 3 commits into from
Nov 3, 2022

Conversation

fangerer
Copy link
Contributor

This PR add a bunch of new API functions for deleting items and attributes (resolves issue #369).

Similar to CPython, functions HPy_DelAttr(_s) are just inline helper functions that use HPy_SetAttr(_s) and pass HPy_NULL as value.

For HPy_DelItem(_i/_s), I followed the same strategy as we did for HPy_SetItem(_i/_s).

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me. The asymmetry between delitem and delattr is a bit odd, but I think this is just part of Python now. It would be nice to split the setattr and delattr tests so that each test is simpler, but perhaps there is a good reason not to do that?

@@ -183,7 +183,7 @@ def foo(self):
assert mod.f(PropAttr()) is True
assert mod.f(PropAttrRaising()) is False

def test_setattr(self):
def test_setattr_delattr(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the underlying implementation of delattr uses setattr, but could we keep the tests separate just so that each test function is a bit simpler?

Copy link
Contributor

Choose a reason for hiding this comment

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

And the same comment for the other setattr / delattr tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can do that. I just merged them into one test because setattr/-item are obviously related to delattr/-item and in this way, we don't generate a second test module which is faster.
But I don't really mind. I'll separate it.

Copy link
Collaborator

@antocuni antocuni left a comment

Choose a reason for hiding this comment

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

LGTM.
I think that this PR is mergeable as it, thank you!
I'm writing some random comments just for the sake of the conversation, but they shouldn't prevent you to merge:

  • these tests are very comprehensive: sometimes it is hard to distinguish whether we are testing HPy (i.e. "HPy_DelAttr" should delete the attribute) or the underlying python implementation, not HPy-specific (i.e. "the logic for deleting attributes has this and that corner cases" -- you could write the same test in pure python). I think it's a common problem of our test suite, we don't have a standard way to decide which parts of the semantics are HPy-specific and which one are python-specific.

  • semi-related to the previous point, the tests have a lot a duplication: e.g. test_setattr and test_setattr_s are basically the same. But we don't have a good way to merge the two, unless we start using complicate fixtures and/or templating magic (but I think we are already doing too much templating magic in our tests). Maybe one solution could be to have a "full" test for test_setattr which covers all the corner cases and a simpler one for test_setattr_s which covers just the simple case? Again, we need to decide whether we are testing HPy or the python implementation

@fangerer
Copy link
Contributor Author

@antocuni

sometimes it is hard to distinguish whether we are testing HPy (i.e. "HPy_DelAttr" should delete the attribute) or the underlying python implementation

Agreed. One source for this problem is that we still try to map HPy API to C API if we can. We should probably define the semantics separately for HPy but then it might happen that we can no longer directly map to a C API.

the tests have a lot a duplication

We could use a template for those kinds of tests and replace the types. I'm also fine with merging them.

@fangerer
Copy link
Contributor Author

@hodgestar please do another review round. I've addressed all your comments.

@antocuni
Copy link
Collaborator

Agreed. One source for this problem is that we still try to map HPy API to C API if we can. We should probably define the semantics separately for HPy but then it might happen that we can no longer directly map to a C API.

I'm not sure to understand. Even if we say that HPy_DelItem maps to PyObject_DelItem, there is no point in testing all the possible combinations of items, is there?

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Looks good!

@fangerer fangerer merged commit a371ea7 into hpyproject:master Nov 3, 2022
@fangerer fangerer deleted the fa/delitem_delattr branch November 3, 2022 10:51
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.

3 participants