Hi Luis,<br><br><div class="gmail_quote">On Sun, Dec 11, 2011 at 8:36 PM, Luís Oliveira <span dir="ltr"><<a href="mailto:luismbo@gmail.com" target="_blank">luismbo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hello,<br>
<br>
I went through all the code and here are my review notes. (In org-mode<br>
format.) I suggest we tackle them incrementally, not necessarily in<br>
this order. Meanwhile, I'll be tackling the missing bits in the new<br>
convert-into-foreign-memory. Should we be having this discussion in<br>
cffi-devel btw?<br></blockquote><div><br>Sure. I've copied the list for anyone who's interested. <br><br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* cffi-fsbv.asd<br>
What happens on #-unix?<br>
(:cffi-grovel-file "libffi" :pathname #+unix "libffi-unix")<br></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* fsbv/package.lisp<br>
1. Regarding SIZET, I'd say we can define this type using a<br>
keyword :SIZET.<br></blockquote><div> </div><div>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.<br>
</div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
2. cffi-fsbv calls and hooks into so many of CFFI internals. Should<br>
we just use the CFFI package? I mean, the code is full of ::s.<br></blockquote><div><br>I'm fine with that. I don't think there'll be any symbol conflicts.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* fsbv/libffi-unix.lisp<br>
1. OSX ships with libffi. Is (cc-flags "-I/opt/local/include/")<br>
necessary? (I guess it doesn't hurt. Just curious. Perhaps it<br>
didn't ship with libffi in the past?)<br></blockquote><div><br>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 :-)<br>
<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
2. Aren't :uint and :ushort equivalent to:<br>
(ctype ushort "unsigned short")<br>
(ctype unsigned "unsigned")?<br></blockquote><div> </div><div>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.<br> </div>
<blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
3. Do we really need to grovel this stuff? Grovelling requires a C<br>
compiler which is particularly tricky on Windows. It'd be nice to<br>
avoid that requirement if possible. Didn't see anything in here<br>
that seemed to *require* grovelling. Did I miss something?<br></blockquote><div><br>"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?<br>
<br>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.<br><br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* fsbv/build-in-type.lisp<br>
1. Could we use DEFCVAR here to declare the various ffi_type_*<br>
globals? (I think it'd be slightly clearer.)<br></blockquote><div> </div><div>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.<br>
<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
2. Why do we have to cache pointers for FOREIGN-TYPE-ALIAS and<br>
FOREIGN-ENUM? (But see my comments on fsbv/cstruct.lisp)<br></blockquote><div><br>We probably don't.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
3. Why different LIBFFI-TYPE-POINTER for those two types?<br>
FOREIGN-ENUM inherits from FOREIGN-TYPE-ALIAS.<br></blockquote><div><br>Is it really different, or is it just getting the pointer from the underlying type?<br><br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* fsbv/cstruct.lisp<br>
1. slots-in-order should be moved near FOREIGN-STRUCT-TYPE.<br>
2. I think we should move libffi-type-pointer out of FOREIGN-TYPE<br>
and centralize storage in an hash-table.<br>
a) this simplifies implementation a little bit: it's weird that<br>
pretty much all libffi-type-pointer methods have :AROUND<br>
qualifiers.<br>
b) makes it easy to avoid memory leaks when redefining/reloading<br>
structure types. (Although this sort of leak is probably not a<br>
huge issue, it's certainly inelegant.)<br>
3. The code that creates ffi_types for structs could use some<br>
refactoring. :-) I'll give it a go.<br></blockquote><div><br>Sure.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* fsbv/functions.lisp<br>
1. I think this file can benefit from a little bit of refactoring.<br>
2. I *think* we can move the indirection logic from the translation<br>
methods onto FFCALL-BODY-LIBFFI, but it's not yet clear how that<br>
can happen.<br></blockquote><div><br>Interesting thought, I'd like to hear your ideas.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* fsbv/example.lisp<br>
1. Need to clear the dead code and move it to examples/.<br></blockquote><div><br>examples.lisp? It's probably all dead code now, just get rid of the whole thing.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* src/functions.lisp<br>
1. We can add a neat restart to *foreign-structures-by-value*.<br></blockquote><div><br>Yes I think you mentioned this idea on the todo list. I expect the restart is "load cffi-libffi"?<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* src/structures.lisp<br>
1. As I've mentioned before, we should discuss how we can implement<br>
this sort of functionality in a backwards-compatible way.<br></blockquote><br>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.<br>
<br><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
* src/early-types.lisp<br>
1. missing expand-into-foreign-memory, and all the hooking up that<br>
implies. Started working on that.<br>
2. I think the :indirect keyword is delegating the responsibility to<br>
the wrong place. All the type translations (in CFFI or in other<br>
libraries) would have to be updated accordingly! As mentioned in<br>
the fsbv/functions.lisp notes, I think we can handle the<br>
indirection there. (It might require a slightly different way of<br>
hooking up ffcall-body-libffi with foreign-funcall.)<br></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
* Nomenclature<br>
I can forsee using libffi to deal with stuff like callbacks, long<br>
long, varargs, etc. Should we rename cffi-fsbv to cffi-libffi?<br></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
* Wishlist<br>
*** foreign-funcall-varargs and friends could use libffi<br>
... to do proper varargs.<br>
*** defcallbacks with structures...<br></blockquote><div><br>Yup. <br><br>Liam <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<font color="#888888"><br>
--<br>
Luís Oliveira<br>
<a href="http://r42.eu/%7Eluis/" target="_blank">http://r42.eu/~luis/</a><br>
</font></blockquote></div><br>