Closed rosmo closed 5 years ago
Merging #15 into master will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #15 +/- ##
======================================
Coverage 8.25% 8.25%
======================================
Files 14 14
Lines 4024 4024
======================================
Hits 332 332
Misses 3692 3692
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 1be7d9a...91d49d5. Read the comment docs.
Thanks for the contributions. I'll have to think before I integrate these changes. I was planning a more native go-binding at some point, not using SWIG. Also this introduces a C++ dependency as well.
Regarding testdata, I'm storing it in https://github.com/libyal/testdata
Thanks for considering. I think getting a nicer "native" Go-binding without using C++ and Swig is hard. You'd have to either re-implement the library in Go or create a wrapper using Cgo. My PR doesn't add any new builds/requirements to the library, it's only relevant if one wants to use Go.
My PR doesn't add any new builds/requirements to the library, it's only relevant if one wants to use Go.
is adds a dependency on swig and a C++ compiler. Using C++ also means that now the binding has to deal with both C (malloc/free) and C++ (new/delete) memory allocation.
Where cgo (C.malloc/C.free) seems to be a wrapper around C (malloc/free), also see: https://golang.org/cmd/cgo/
Some more things to consider before going the cgo route https://www.cockroachlabs.com/blog/the-cost-and-complexity-of-cgo/. Sounds like a native go implementation might be the better route to go (pun intended).
I'll close this issue, seeing end result is that the proposed changes complicate maintenance.
These are simple Go bindings for libvmdk created with Swig and some C++ glue.
The test suite requires a simple pre-built VMDK test file (128 KB), not sure if you want to include that in the repository or not? Let me know if I should add it to the PR.