Hi Luis,

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/