On Sun, Dec 11, 2011 at 8:36 PM, Luís Oliveira <luismbo@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/