fix writing large axml chunks

[tomjnixon]

This required a few changes:

• enlarge the JUNK chunk
• fix ds64 check to use ds64 when any chunks are large
• add all large chunks to ds64
• check that JUNK is large enough for ds64

... and some other improvements along the way

• add Bw64Writer::close() to be able to catch exceptions during finalisation
• add tests for large non-data chunks
• allow moving the string into AxmlData, and accessing the string with AxmlData::data() to avoid copies while reading/writing
• add benchmark for AxmlChunk
• remove files after successful [big] tests
• add a basic nix build

[codecov-commenter]

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (5ff824f) 83.45% compared to head (d3dae5c) 83.23%.

Files	Patch %	Lines
include/bw64/writer.hpp	42.30%	15 Missing ⚠️
include/bw64/reader.hpp	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   83.45%   83.23%   -0.22%     
==========================================
  Files           6        6              
  Lines         677      698      +21     
==========================================
+ Hits          565      581      +16     
- Misses        112      117       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[firthm01]

Looks great overall! Just a couple of comments

[firthm01]

Could this flag be factored out by always closing the file even if errors occurred? we're considering any call to this a one-shot thing anyway with no retry. So, e.g, wrap the finalise calls in a try/catch, and close on catch with a pass-through of the exception? Can just use if(fileStream_.is_open()) then for the condition. Perhaps bad practice to "try" though.

[tomjnixon]

I did briefly consider that, and went with this because i didn't really want to think about fstreams too much (e.g. can an error make is_open return false?)

I think this is safe (supposedly is_open is only cleared by close), though maybe messier overall than one more flag. I've pushed a commit that changes this, what do you think?

[firthm01]

I slightly prefer not having the flag, but mainly because the var name didn't really reflect the state. I.e, it's not necessarily closed if closed_ is true - that flags really just being use to prevent repeated attempts to close. So I guess I was more thinking the var name either needs to reflect what's it's doing or made to really track closed state (in which case is_open does that anyway).
I'm unsure whether not closing might cause hanging open files too? it probably gets properly closed in any case when the object is destructed tbh.

[tomjnixon]

Yeah good point, i think I'll stick with the exception based one.

The "closed" naming was just to match the "close" function, which i think makes sense especially for things working with files.

I'm unsure whether not closing might cause hanging open files too? it probably gets properly closed in any case when the object is destructed tbh.

I think it's fine, and works in testing, but I probably add a test of this in the normal set too (the big ones don't get ran in CI).

[tomjnixon]

Looking at it again, it seems odd to have .close() on writers but not readers, but it seems like a good opportunity to check for any file errors that haven't been spotted anyway, so i've added that. Sorry for the spam :)