(H5PT) Improved Append() Method

[donkahlero]

To keep the fun alive :sunglasses:

This patch features an improvement of the HDF5 PacketTable Append method. This was necessary, because the old version did not support the append of struct containing nested structs and/or slices/arrays. To sum it up:

• Any data of any Compound Datatype can be written to a PacketTable now.
• This is achieved by allocating memory that contains the necessary data.
• The whole code is simpler than the doubled code that excisted before.
• The Append() method now supports proper appending of slices, which are not appended in a append-per-member manner, but rather as several objects to append that are consecutively in memory. This is more aligned with the offical way-of-doing.

What needs to be improved?

• The append of pointers still follows the old notion that we derefence the pointer and write the value directly. To support H5T_STD_REF_OBJ, further, significant changes have to be done to the whole code base.
• The read functionality has to be revised, as it seems to not work when strings are part of compound datatypes. Manual testing has shown that they are written properly, the test cases still would fail, because of the mentioned call to read.

I suggest that the above mentioned improvements are tackled in a separate patch.

[sbinet]

Thanks :) I'll have a look at it on Monday (when I come back from holidays)

[donkahlero]

Yes, take your time :) I guess you will have numerous comments :sunglasses:

[donkahlero]

Hi @sbinet, yes you are right. It is not the most beautiful way of handling this. But for me it seems the best way of handling it ATM. Obviously, revising the code is always an option :)

Nevertheless, we are using the code quite successfully here for some huge datasets (appending 5MB+ a time at a high rate). And so far Append() was stable. I hope this makes you a bit more comfortable to hear :)

[sbinet]

yes, my comment (wrt a CMarshal (?) method) was more a side comment rather than a road blocker.

[donkahlero]

Alright! But definitely something to keep in mind! :sunglasses: Other than that - improved the code based on your comments :)

[donkahlero]

Actually there is one thing @sbinet. It just came to my mind that I have no case for uint and int. Do you want me to add one that is then just a uint64 and int64? Any other changes that you want for this PR?

[sbinet]

a Go int has the size of int64 only on a 64b machine. this would make the creation of an HDF5 file non portable...

I would err on the side of just returning an error. (with a CMarshal version we could actually create a []byte with int64 data w/o fiddling w/ alignment+offset)

[donkahlero]

Okay. Then we can keep it for now? I guess so, but I would need to do that another time and PR :smiley:

[sbinet]

LGTM but could you reword a bit the commit message so it reads something like:

hdf5: improve Table.Append bla-bla

bla-bla foo bar
qux and baz

[donkahlero]

Done, sir :)

[sbinet]

thanks! let's wait a bit for @kortschak to chime in :)

[donkahlero]

Hi @kortschak!

Thanks for your nice comments! I think I have fixed all the issues that you have outlined.

A comment in regard to naked returns, etc pp.: maybe I could create a P/R in regard to a feasible linter setup, so the Travis check would actually fail if the code does not look a certain way? Of course we would need to agree on a proper set of linters + this would eventually lead to some fixing of the source (which should not be an issue after all).

[kortschak]

If there's a linter for naked returns that might be worth running, but it's not a clear cut case of always avoiding them, so incorporating it in the build is probably more trouble than it's worth.

[donkahlero]

I think a linting setup in general would improve the code of the whole lib. Maybe just add golint? It doesn't have to be gometalinter with a full stack of linters :P

[donkahlero]

Haha, great that this fails - has nothing to do with the code, I guess?

[kortschak]

I've restarted the failing test.

My experience with linting in builds has not been good. Vetting is OK (and now mandatory), but linting involves too much style.

[donkahlero]

Hi @sbinet,

added the nil for bot the ad.ptr and each string. I am actually not quite sure that this can happen, as the pointer that is passed to C.H5PTappend (in case of appending a single string only) will point to an address that will then contain the address of the string to append.