[cffi-devel] First stab at reviewing cffi-fsbv

[Liam Healy]

Hi Luis,

On Sun, Dec 11, 2011 at 8:36 PM, Luís Oliveira <luismbo at gmail.com> wrote:

> Hello,
>
> I went through all the code and here are my review notes. (In org-mode
> format.) I suggest we tackle them incrementally, not necessarily in
> this order. Meanwhile, I'll be tackling the missing bits in the new
> convert-into-foreign-memory. Should we be having this discussion in
> cffi-devel btw?
>

Sure.  I've copied the list for anyone who's interested.


> * cffi-fsbv.asd
>  What happens on #-unix?
>  (:cffi-grovel-file "libffi" :pathname #+unix "libffi-unix")
>

Absolutely nothing.  Someone named "CRLF0710" sent me an email claiming to
have a libffi-win32 for the old standalone version of FSBV, and I wrote
back saying I was interested but I've heard nothing.


> * fsbv/package.lisp
>  1. Regarding SIZET, I'd say we can define this type using a
>     keyword :SIZET.
>

Yes, if you think that's OK.  That was me totally sneaking something in
that GSLL needed that was awkward to generate there but that CFFI could
generate easily.


>  2. cffi-fsbv calls and hooks into so many of CFFI internals. Should
>     we just use the CFFI package? I mean, the code is full of ::s.
>

I'm fine with that.  I don't think there'll be any symbol conflicts.


>
> * fsbv/libffi-unix.lisp
>  1. OSX ships with libffi. Is (cc-flags "-I/opt/local/include/")
>     necessary? (I guess it doesn't hurt. Just curious. Perhaps it
>     didn't ship with libffi in the past?)
>

Ugh, OSX.  It apparently does ship with libffi, except when it doesn't.
There are so many ways to install system software, I just gave up.  I'm all
for your approach because it does simplify the code and make it match the
other OSes; when the complaints come in to cffi-devel, you can answer them
:-)


>
>  2. Aren't :uint and :ushort equivalent to:
>       (ctype ushort "unsigned short")
>       (ctype unsigned "unsigned")?
>

Um, I don't know?  What are you advocating here?  Removing something that I
put in?  :uint and :ushort don't ring a bell for me.


>
>  3. Do we really need to grovel this stuff? Grovelling requires a C
>     compiler which is particularly tricky on Windows. It'd be nice to
>     avoid that requirement if possible. Didn't see anything in here
>     that seemed to *require* grovelling. Did I miss something?
>

"Here" means libffi-unix?  The constants at the end aren't necessary, but I
think the rest is.  In particular, FFI_DEFAULT_ABI (abi) and FFI_OK
(status) are pretty critical, as is the struct ffi-type.  I'm a little
confused; I thought the point of cffi-grovel was to provide access to
definitions in .h files, and that's what we need for ffi.h and
ffi-target.h.  How do you propose defining these without groveling?  Just
hardwiring into lisp and hoping no one ever changes those values?

By the way I have FFI_STDCALL commented out and didn't try to mate it with
CFFI's definition on Windows, perhaps CRLF0710 or another Windows user can
figure that out.


> * fsbv/build-in-type.lisp
>  1. Could we use DEFCVAR here to declare the various ffi_type_*
>     globals? (I think it'd be slightly clearer.)
>

I kind of like the automatic way of doing it; manually means that every
time something changes (new type added or old one removed) you have to
remember to do it in two places.  That's a recipe for breakage.


>  2. Why do we have to cache pointers for FOREIGN-TYPE-ALIAS and
>     FOREIGN-ENUM? (But see my comments on fsbv/cstruct.lisp)
>

We probably don't.


>  3. Why different LIBFFI-TYPE-POINTER for those two types?
>     FOREIGN-ENUM inherits from FOREIGN-TYPE-ALIAS.
>

Is it really different, or is it just getting the pointer from the
underlying type?


> * fsbv/cstruct.lisp
>  1. slots-in-order should be moved near FOREIGN-STRUCT-TYPE.
>  2. I think we should move libffi-type-pointer out of FOREIGN-TYPE
>     and centralize storage in an hash-table.
>     a) this simplifies implementation a little bit: it's weird that
>        pretty much all libffi-type-pointer methods have :AROUND
>        qualifiers.
>     b) makes it easy to avoid memory leaks when redefining/reloading
>        structure types. (Although this sort of leak is probably not a
>        huge issue, it's certainly inelegant.)
>  3. The code that creates ffi_types for structs could use some
>     refactoring. :-) I'll give it a go.
>

Sure.


>
> * fsbv/functions.lisp
>  1. I think this file can benefit from a little bit of refactoring.
>  2. I *think* we can move the indirection logic from the translation
>     methods onto FFCALL-BODY-LIBFFI, but it's not yet clear how that
>     can happen.
>

Interesting thought, I'd like to hear your ideas.


>
> * fsbv/example.lisp
>  1. Need to clear the dead code and move it to examples/.
>

examples.lisp?  It's probably all dead code now, just get rid of the whole
thing.


>
> * src/functions.lisp
>  1. We can add a neat restart to *foreign-structures-by-value*.
>

Yes I think you mentioned this idea on the todo list.  I expect the restart
is "load cffi-libffi"?


>
> * src/structures.lisp
>  1. As I've mentioned before, we should discuss how we can implement
>     this sort of functionality in a backwards-compatible way.
>

I'd prefer not defaulting to previous assumptions (that is, no translation
of structures), if that's what backwards-compatible means.  But I'm
interested in how you'd approach this.

* src/early-types.lisp
>  1. missing expand-into-foreign-memory, and all the hooking up that
>     implies. Started working on that.
>  2. I think the :indirect keyword is delegating the responsibility to
>     the wrong place. All the type translations (in CFFI or in other
>     libraries) would have to be updated accordingly! As mentioned in
>     the fsbv/functions.lisp notes, I think we can handle the
>     indirection there. (It might require a slightly different way of
>     hooking up ffcall-body-libffi with foreign-funcall.)
>

No doubt.  I would like to avoid special-casing though, e.g. "here's an
enum, got to indirect it".  I think that kind of thing belongs in a method
associated with the type.


> * Nomenclature
>  I can forsee using libffi to deal with stuff like callbacks, long
>  long, varargs, etc. Should we rename cffi-fsbv to cffi-libffi?
>

Sure.  Also, you mentioned that some implementations have their own
call-by-value mechanism, so it makes more sense to rename it, since they
would have "fsbv" but no libffi.


>
> * Wishlist
> *** foreign-funcall-varargs and friends could use libffi
>    ... to do proper varargs.
> *** defcallbacks with structures...
>

Yup.

Liam

>
> --
> Luís Oliveira
> http://r42.eu/~luis/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.common-lisp.net/pipermail/cffi-devel/attachments/20111212/cc3fa8d9/attachment.html>