Wrong buffer allocation and encoding compare

[NINI1988]

The call to var buffer=Buffer.alloc(8*10*8, "binary"); is wrong because the second argument is the fill value not the encoding. So the encoding is never set in the buffer.

In the function make_dataset_from_buffer

if (!std::strcmp("binary", (*encoding))) {
  herr_t err = H5LTmake_dataset_string(group_id, dset_name, const_cast<char*>(node::Buffer::Data(buffer)));
  if (err < 0) {
    v8::Isolate::GetCurrent()->ThrowException(
      v8::Exception::SyntaxError(String::NewFromUtf8(v8::Isolate::GetCurrent(), "failed to make char dataset")));
      return;
  }
return;
}

This makes a string dataset when the encoding is set to binary, not the other way around.

[rimmartin]

hmmm, this was changed from the deprecated new Buffer constructors but not correctly.

Wonder if the fill can be a 0:

var buffer=Buffer.alloc(8*10*8, "\0", "binary");

will change and test

[NINI1988]

var buffer=Buffer.alloc(8*10*8) worked for me.

Also if (!std::strcmp("binary", (*encoding))) { means when encoding is equals binary. But the expression should be inverted.

[rimmartin]

binary is supposed to equal "encoding"?

I'l test

Commit another bug fix and the "\0" in tests

[NINI1988]

But when encoding is binary then H5LTmake_dataset_string will be called.

Example: When encoding is binary then std::strcmp("binary", (*encoding) -> 0 !0 -> 1 H5LTmake_dataset_string called

[rimmartin]

https://github.com/HDF-NI/hdf5.node/issues/20 in older version the native Buffer was it's own custom thing till v8 changed something. Then nodejs people went to using Int8Array as their Buffer. The H5LTmake_dataset_string doesn't make an actual string but handles chars which int8 is; this use was from way back when I used all hdf5 HL commands but then realized they were more limited and started adding in all the low level calls.

I'm wondering if this can be removed and let it just go to the remain code. I'll experiment. Not sure if there is any user using nodejs before v4.2.x. With any change on this I'll tag the repository so I can attempt to help such a user if they exist

[rimmartin]

Also the code should not have new and delete in it ; rather use the c++ memory management. I'll go thru this evening and get these

[rimmartin]

Ah which version of nodejs are you using?

They changed the meaning of

node::Buffer::HasInstance

again!

This is why std::strcmp("binary", (*encoding) is acting strange for you and now me. Only supposed to be in make_dataset_from_buffer if it's an wrapper of Int8Array. It's going there now for all typed arrays.

Going to have to reorder things and find how old a version of nodejs we can support on new release

[NINI1988]

I'm using v8.11.2.

[rimmartin]

I have more unit tests back to working with nodejs v9.9.0. I'll be testing back thru versions including yours. Some of the unit tests I have running with

{rank: 2, rows: numberOfDataLines, columns: 3} option style. We'll see what the day brings

[NINI1988]

Ok, great.

[rimmartin]

tests: 66 passing (434ms) 8 pending

The pending ones are ones skipped because they depend on h5's people gave me locally to test. I want to replace with ones the tests create on the fly; don't want to clutter repository or use something someone never gave permission for such use.

testing with different versions of nodejs...

[rimmartin]

After a release and tagging with the deprecation, the reserved properties will go away for v4.x.x