hildjj / node-cbor

Encode and decode CBOR documents, with both easy mode, streaming mode, and SAX-style evented mode.
MIT License
357 stars 73 forks source link

Support serialising of single precision float outside of canonical #97

Closed misos1 closed 5 years ago

misos1 commented 5 years ago

I understand that handling half precision floats can be expensive as there is scarce native hardware support so it is hidden behind canonical. But I am guessing that it should not be so slow with single precision floats.

So why not do it in this way:

  _pushFloat(obj) {
    if (this.canonical) {
      // TODO: is this enough slower to hide behind canonical?
      // It's certainly enough of a hack (see utils.parseHalf)

      // From section 3.9:
      // If a protocol allows for IEEE floats, then additional canonicalization
      // rules might need to be added.  One example rule might be to have all
      // floats start as a 64-bit float, then do a test conversion to a 32-bit
      // float; if the result is the same numeric value, use the shorter value
      // and repeat the process with a test conversion to a 16-bit float.  (This
      // rule selects 16-bit float for positive and negative Infinity as well.)

      // which seems pretty much backwards to me.
      const b2 = Buffer.allocUnsafe(2)
      if (utils.writeHalf(b2, obj)) {
        if (utils.parseHalf(b2) === obj) {
          return this._pushUInt8(HALF) && this.push(b2)
        }
      }
    }

    const b4 = Buffer.allocUnsafe(4)
    b4.writeFloatBE(obj, 0)
    if (b4.readFloatBE(0) === obj) {
      return this._pushUInt8(FLOAT) && this.push(b4)
    }

    return this._pushUInt8(DOUBLE) && this._pushDoubleBE(obj)
  }

Would not be Math.fround useful here?

hildjj commented 5 years ago

I like the Math.fround approach. I'm going to post a PR here for comment for a couple of days before committing it and cutting a release. I think that since this changes the output of the system, this is at least a minor version bump. Opinions?

Also, in my opinion, Float32Array and Float64Array should always generate floats of that size, not something smaller -- so I've made that fix here as well.