vapor-community / node

A formatted data encapsulation meant to facilitate the transformation from one object to another
MIT License
25 stars 20 forks source link

Explicitly use int64 value of StructuredData #93

Open koraykoska opened 7 years ago

koraykoska commented 7 years ago

Currently StructuredData.Number is limited to int, uint and double, which is perfectly fine for people who use 32 bit integers and 64 bit integers on 64 bit platforms.

But if you are on a 32 bit platform and you expect aStructuredData value to be e.g. an unsigned 64 bit integer, you will probably have some unexpected silent problems as Int and UInt are limited to 32 bits andStructuredData will return Int.max for integers bigger than 32 bits.

I am currently working on a vapor library for Telegram chatbots and they have stated in their documentation that some values are actually bigger than 32 bits which means I must ignore 32 bit platforms and add a warning to the README that this library should not be used on 32 bit platforms.

What do you guys think about that? Do we want to ignore 32 bit platforms as they already become extinct or do we want to add support for explicit int64 and uint64 values inside StructuredData.Number?

Reference (Number.swift file in this library): Number.swift

tanner0101 commented 7 years ago

If an application requires native integers to be 64 bits in width then I think it makes sense to not allow that application to run on 32 bit machines. I don't know exactly how Int64 works on a 32 bit machine (does it?). If it does, there's probably a considerable performance impact at least.

vzsg commented 7 years ago

Int64 works just fine on 32 bit systems too. Consider C, the long long type has been available since C99, years before 64 bit systems became generally available 😛.

Indeed, there might be performance penalty involved, but some applications really require 64 bit integers.

tanner0101 commented 7 years ago

tl;dr: use strings for now, this issue raises deeper questions about the viability of StructuredData that need to be addressed.


@vzsg good point. :)

As a workaround for now, 64 bit integers could be stored as strings in the structured data and converted to 64 bit integers in an extension.

I think this issue will be made easier to tackle in upcoming updates to Vapor that will make the StructuredData type much less prevalent.

@Ybrin what type specifically are you using in this case? Are you using a Node or StructuredData directly or are you using one of the sub types like Row or JSON? In 3.0 we're hoping to transition the subtypes to being their own types not reliant on StructureData. This means that it would be much easier to add say Int64 support to just Row if it's needed there.

Right now StructuredData is a bit of a god object. Changing it even slightly affects a ton of code. For example, JSON currently extends StructuredData and all JSON can just be Doubles. Adding Int64 support there (or having Int support in the first place) could potentially cause interoperability problems, unexpected behavior, or unnecessary overhead .