git_strarray marshaling improvements

[phkelley]

I noticed that GitStrArrayOut's Dispose method was implemented incorrectly -- fixing that led me down this path towards trying to unify our GitStrArray types and clean things up. We still end up with two different types, GitStrArrayManaged and GitStrArrayNative, which correspond to the old In and Out types, respectively. But this PR switches to structs, gives a bit more code re-use, fixes some bugs, and manages to eliminate the UnSafeNativeMethods class entirely!

I'm eager to get the Mono results...

[nulltoken]

Couldn't we somehow leverage LaxUtf8Marshaler.FromNative() here?

[phkelley]

I can do that. I think at the time I was writing it I felt like it was weird to mix the policy of the marshaler itself (its "Lax-ness") with what I was doing here, which doesn't have anything to do with the marshaler's policy other than its choice of an encoding.

[phkelley]

What's up with Travis?

[ethomson]

Dead: http://status.travis-ci.com/

On Tue, Jun 24, 2014 at 6:46 PM, Philip Kelley [email protected]
wrote:

What's up with Travis?

—
Reply to this email directly or view it on GitHub
#773 (comment).

[nulltoken]

👍 for me.

@Therzok How do you like this?

[Therzok]

An using could be used here.

[Therzok]

Other than using using wherever appropriate, this looks awesome!

[Therzok]

use var toReturn = new GitStrArrayManaged() ?

[Therzok]

I think I missed a few var/using usages towards the end, just because I didn't want to spam them all. I'm really happy to merge this once those nitpicks are done. (Clean code is even better than better code. 👍)

[nulltoken]

switching the try\catch to the using statement doesn't make the compiler happy 😿

error CS1655: Cannot pass fields of 'array' as a ref or out argument because it
is a 'using variable'
error CS1656: Cannot assign to 'array' because it is a 'using variable'

[nulltoken]

Fixed the vars

[Therzok]

Oh, I see the try/finally thing now. We're just allocating the memory there then we're passing the structure. Oh well, try/finally it is then.

[phkelley]

Thanks guys for the fixups. Yes, we can't make use of "using" syntax here which is unfortunate. It's the price of using structs, but I think we really have to have the structs because of the way that GitStrArray and friends end up embedded inside of other types like GitDiffOptions.

I didn't know we had standardized on 'var' syntax everywhere. I'll keep that in mind for future changes.

The Array = new GitStrArray(); lines are basically just a safety mechanism to protect against double-dispose being a double-free. They just set the Strings and Count members to zero so that on any re-run of the Dispose method, nothing happens. I agree that the caller should only ever call Dispose() once. Let me know what you think on that.

[nulltoken]

I didn't know we had standardized on 'var' syntax everywhere. I'll keep that in mind for future changes.

Actually, it's not var everywhere. If the type is known on the right hand side, we use var. Otherwise, we don't.

var l = new List<int>();
var c = (int)count;
MyStronglyType mst = Build();

[nulltoken]

@phkelley 💟