Hi Luis, On Sun, Dec 11, 2011 at 8:36 PM, Luís Oliveira <luis...@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/ >
_______________________________________________ cffi-devel mailing list cffi-devel@common-lisp.net http://lists.common-lisp.net/cgi-bin/mailman/listinfo/cffi-devel
