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

Enhance "wrapping C libraries". #2362

Merged
merged 3 commits into from
Jun 20, 2018

Conversation

gabrieldemarmiesse
Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse commented Jun 16, 2018

  • Used intptr_t instead of int to avoid warnings of unsafe cast (and crashes since C++ consider casting from void* to int an error).
  • Moved the main implementation of the queue into the examples directory for testing.
  • Removed the old examples files (they were not used anyway), all the equivalents are in examples/tutorial/clibraries.

@scoder
Copy link
Contributor

scoder commented Jun 17, 2018

Well, there is the "little" issue that MSVC lacks stdint.h, so it would be nice to have the example get along without it. Maybe Py_ssize_t would work as well?

Also, using intptr_t in the cdef method signatures is really unhelpful as the idea is to pass an int arrray as an additional C-level interface. Better restrict the specific integer types to casts and leave everything else with int to keep the external interface clean.


/* Pop values as long as the predicate evaluates to true for them,
* returns -1 if the predicate failed with an error and 0 otherwise.
*/
int queue_pop_head_until(Queue *queue, predicate_func predicate,
intptr_t queue_pop_head_until(Queue *queue, predicate_func predicate,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but this is really not what the C-API of the queue looks like.

@gabrieldemarmiesse
Copy link
Contributor Author

It's true that it's a lot cleaner now. My experience with plain C is somewhat limited, as you can see. It's getter better with your reviews.

# method, as the method signature is incompatible with Python argument
# types (Python doesn't have pointers). However, we can make a method
# called `extend_python` instead that accepts an arbitrary Python iterable.
cpdef extend_python(self, values):
Copy link
Contributor

@scoder scoder Jun 17, 2018

Choose a reason for hiding this comment

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

The naming of these two methods was intentional. The Python interface dominates the API of this class and should be consistent. extend() is the method name a Python user would expect. The additional C-interface should be a second class citizen wherever it cannot match the Python interface.

Maybe we could rename the cdef extend() method to extend_ints(). That way, we could easily add an extend_longs() or extend_chars() method for convenience, as one would do in C APIs.

@scoder scoder merged commit 88034bd into cython:master Jun 20, 2018
scoder added a commit that referenced this pull request Jun 20, 2018
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.

None yet

2 participants