Created attachment 201009 [details] [review]
wordexp and fnmatch bindings

Review of attachment 201009 [details] [review]:

Thanks for the patch. The posix vapi usually tries to not change the name of the constants (FNM_NOMATCH in this case) and try to not use enums but rather const int. 
Also it's not correct to use we_wordc as length for the array, because if we_offs is used then the real length is we_wordc+we_offs. So it must be an array without length from a Vala view point unfortunately.

(In reply to comment #2)
> Thanks for the patch. The posix vapi usually tries to not change the name of
> the constants (FNM_NOMATCH in this case) and try to not use enums but rather
> const int.

Okay. I was trying to be typesafe. ;-) Should I put WRDE_x as just x inside struct word?

> Also it's not correct to use we_wordc as length for the array, because if
> we_offs is used then the real length is we_wordc+we_offs. So it must be an
> array without length from a Vala view point unfortunately.

Actually, I realized there is no point to having we_offs at all. we_offs must be set by the user before wordexp is called with WRDE_DOOFFS, which isn't possible with it being an out parameter. So, I could change it to ref and allow this or leave it an drop WRDE_DOOFFS. Which is preferred? Is it possible to make this work via a array_length_cexpr? (It seems like what that's for, but I've not found sample code using it.)

(In reply to comment #3)
> Okay. I was trying to be typesafe. ;-) Should I put WRDE_x as just x inside
> struct word?

WRDE should be fine.

> Actually, I realized there is no point to having we_offs at all. we_offs must
> be set by the user before wordexp is called with WRDE_DOOFFS, which isn't
> possible with it being an out parameter. So, I could change it to ref and allow
> this or leave it an drop WRDE_DOOFFS. Which is preferred? Is it possible to
> make this work via a array_length_cexpr? (It seems like what that's for, but
> I've not found sample code using it.)

Try making it an instance method with CCode (instance_pos = 1.1) or such (see Glob for reference).

(In reply to comment #4)
> Try making it an instance method with CCode (instance_pos = 1.1) or such (see
> Glob for reference).

Okay, but if you call it multiple times, you leak memory. In the case of wordexp, you can use WRDE_REUSE to realloc. glob leaks memory as it is codified now.

Try:
var g = Posix.Glob();
g.glob("*.vala");
g.glob("*.vala");

and valgrind says:
==16064== 53 (24 direct, 29 indirect) bytes in 1 blocks are definitely lost in loss record 4 of 4
==16064==    at 0x4C28F9F: malloc (vg_replace_malloc.c:236)
==16064==    by 0x4C29019: realloc (vg_replace_malloc.c:525)
==16064==    by 0x4EDEF62: glob_in_dir (glob.c:1458)
==16064==    by 0x4EE02C7: glob (glob.c:1008)
==16064==    by 0x4009F6: _vala_main (in /home/andre/Documents/Coding/vala/wrde)
==16064==    by 0x400A57: main (in /home/andre/Documents/Coding/vala/wrde)

That's true. It is weird api in either ways. The offset is useful for adding stuff before in C, but maybe in Vala it doesn't apply and we could drop offsets.

(In reply to comment #5)
> Okay, but if you call it multiple times, you leak memory. In the case of
> wordexp, you can use WRDE_REUSE to realloc. glob leaks memory as it is codified
> now.
> 
> Try:
> var g = Posix.Glob();
> g.glob("*.vala");
> g.glob("*.vala");

I forgot to say in glob it's possible to use GLOB_APPEND. In which case that works correctly.

O(In reply to comment #7)
> (In reply to comment #5)
> > Okay, but if you call it multiple times, you leak memory. In the case of
> > wordexp, you can use WRDE_REUSE to realloc. glob leaks memory as it is codified
> > now.
> I forgot to say in glob it's possible to use GLOB_APPEND. In which case that
> works correctly.

Okay. What about this:
Make wordexp and glob be private methods with wrappers that have the appropriate flags. Something like:

struct word {
private int _wordexp(string pattern, word w, int flags);
public int again(string pattern, int flags) {
return _wordexp(pattern, this, flags | REUSE);
}

Though, that's kind of pointless for wordexp since calling it as an out parameter will call wordfree. Glob should probably be changed to be similar to the above.

I'm in favour of dropping the offsets.

Created attachment 201251 [details] [review]
wordexp patch that can't leak memory

The new version should be more straight forward to use and won't leak memory in that way. However, I haven't found a good solution for glob. I tried the same thing, but because the GlobErrorFunc has no cname, there is no obvious solution.