Skip to content
This repository was archived by the owner on Oct 24, 2025. It is now read-only.

Conversation

@amcgregor
Copy link
Contributor

Wanted to submit this for some initial review; this is the first chunk of CAPI extension I’ve touched. It’s not complete by far, the test will likely explode and eat someone’s cat, etc. There are some comments in the code so far, including a possible memory leak. Other to-dos include:

  • Documenting what version of libsass is required.
  • Mentioning the version of this package the argument is added.
  • Actually wiring it in.

Work on #81.

pysass.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

py_args leaks here

@asottile
Copy link
Member

I'll be out on holiday for a few days so I probably won't be able to look at this until monday, but it is looking pretty good so far :)

@asottile
Copy link
Member

Documenting what version of libsass is required.

You won't need to bother with this, we statically compile their object files in a pinned submodule

@amcgregor
Copy link
Contributor Author

Thanks for the feedback so far! I had already fixed the py_args leak locally a little bit earlier today, and realized the static submodule thing after getting some sleep. Have a great holiday!

Now increments the reference count for the current list item, as it
isn’t auto-retained by `PyList_GET_ITEM`.

Additionally, corrected missing DECREF to prevent leak on early exit
due to error.
Remarkably, this actually compiles.  I’m almost guaranteed missing
post-compile cleanup involving releasing the function reference
“cookies”.
Also shows what was emitted in the event of a failure.
@amcgregor
Copy link
Contributor Author

Here's another push of updates. Compiles and seems to execute (given my liberal use of the printf school of debugging), with the C side of the callback correctly reporting on the values returned by the callback.

It just doesn't do anything, and I'm not sure why.

@amcgregor
Copy link
Contributor Author

And of course, as soon as I push the value of actual in the tests suddenly reports actually working. >_< I'll tweak and re-push.

@amcgregor
Copy link
Contributor Author

Heh. Now things get fun. Works flawlessly on pypy, py26, and py27. Explodes terribly on pypy3, py33, and py34, but pypy3 actually gives very helpful error reporting! On to making this work on Python 3…

Turns out, it’s also more flexible this way.
@amcgregor
Copy link
Contributor Author

Well there we go. This is now beyond my ability to verify things like memory leaks, if any, however it is operational in all tested environments.

pysass.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can probably use https://docs.python.org/2/c-api/object.html#c.PyObject_CallFunction instead:

PyObject_CallObject(pyfunc, PySass_IF_PY3("y", "s"), py_path)

This'll avoid the complexity of the tuple (the only reason I didn't do this for custom functions is there are a variable number of arguments)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question I have for that is can a class instance with a __call__ method be executed by PyObject_CallFunction vs. PyObject_CallObject?

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure they all go through __call__:

(at least because this works):

>>> def f(a): print(a)
... 
>>> f.__call__
<method-wrapper '__call__' of function object at 0x7f7419588668>
>>> f.__call__(2)
2

Copy link
Member

Choose a reason for hiding this comment

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

introduced some redspace here. I should really add http://pre-commit.com to this project at some point :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, we diverge on style guides in a few interesting ways, and yeah, "redspace" is one of them. I will, of course, ensure my changes conform to the local package style in the end.

@amcgregor
Copy link
Contributor Author

Awesome, thanks for the feedback! (I hope your holiday was restful.) I had been carefully picking around those fprintf lines, but knew I was going to accidentally include a few. (There's about 40 more uncommitted local fprintfs I've been using for debugging. ;) I'll get the items discussed so far cleaned up tonight.

@amcgregor
Copy link
Contributor Author

I do not appear to have the time to really tackle this, nor the CAPI knowledge to do it justice. Is there a tip jar I could throw things at? ;)

@amcgregor
Copy link
Contributor Author

@asottile I wasn't joking about the tip jar, BTW. I have a need for this to work, and can't do it myself at the moment. I feel it's fair to compensate you for assistance in getting this rolled out.

@asottile
Copy link
Member

asottile commented Dec 8, 2015

I was going to take a stab at it tonight. I'll let you know how it goes :)

@asottile
Copy link
Member

asottile commented Dec 9, 2015

I squashed your commits and made one on top here: #110

Please double-check my edits and I'll double check them as well in the morning :)

@asottile
Copy link
Member

#110

@asottile asottile closed this Dec 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants