Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change struct & sjson to use integers. This is slightly more complex #3222

Merged
merged 7 commits into from
Aug 23, 2020

Conversation

pjsg
Copy link
Member

@pjsg pjsg commented Jul 17, 2020

Fixes #3217.

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

This is slightly more complex as we have to avoid casting to lua_Numbers if there a need to have integer values. We also need to deal with unsigned 32 bit integers that are not representable as a lua integer. However, this seems to work

…ve to deal with Unsigned 32-bit integers (that aren't lua integers)
@nwf
Copy link
Member

nwf commented Jul 18, 2020

Color me skeptical about the use of double here; it's going to be implicitly converted to a lua_Number, and that seems like a recipe for loss of precision.

More nit-picky: please use INT_MIN and INT_MAX constants rather than hex literals.

@pjsg
Copy link
Member Author

pjsg commented Jul 18, 2020

The use of double was deliberate as this function returns an integer that can be much larger than a 32 bit integer (which also be unsigned). The double can hold a 53 bit integer -- but it could be that we want this to return a int64_t instead -- but I don't know the platform supports that.

I'll test it.

And yes, I'll fix the INT_MIN/MAX case.

@nwf
Copy link
Member

nwf commented Jul 18, 2020

Ah, I misread the diff in my skim, sorry... double will probably work, but int64_t should work (I make heavy use of the 64-bit int types in sntppkt and they seem to work fine) and will be (very slightly) faster, I think.

@pjsg
Copy link
Member Author

pjsg commented Jul 18, 2020

Fixed sjson as well for the gross effects. Still not sure whether it uses the right number of digits or not.

@pjsg pjsg changed the title Change struct to use integers. This is slightly more complex Change struct & sjson to use integers. This is slightly more complex Jul 18, 2020
lua_pushlstring(data->L, start, end - start);
// See if there is any chance that this is an integer
#if LUA_VERSION_NUM >= 503
char notint = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I discussed in #3213, the Lua 5.3 API already has a function lua_stringtonumber() which does all of this as a standard Lua API call. Why reimplement standard functionality?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short answer is that I did not know about this function....

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjsg TBH Philip, the only reason that I came across this was because of my work to harmonise the 5.1 and 5.3 APIs so that we don't need to have conditional LUA_VERSION_NUM variant code in our modules. In general Lua hides sub types in its exposed API, so there is no difference from an API perspective between the various function subtypes: they are all classed as function. Ditty short vs long strings, and Tables vs ROTables. The one exception is the handling of integer vs number subtypes as there is an lua_isinteger() call and variant get and put API calls. This one is a sensible extension one one that IMO is also worth back-porting into the Lua API. If I do this, then I will also remove this variant code as the 503 version will work fine with both versions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is ugly -- and made worse by the fact that the input buffer is not null terminated and I was nervous that the number parsing might fall off the end of the buffer... I suspect that it would never happen, but I prefer to err on the side of caution.

app/modules/sjson.c Outdated Show resolved Hide resolved
@TerryE TerryE merged commit 0e02c0e into nodemcu:dev Aug 23, 2020
@marcelstoer marcelstoer added this to the Next release milestone Aug 24, 2020
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Nov 21, 2020
…odemcu#3222)

* Change struct to use integers. This is slightly more complex as we have to deal with Unsigned 32-bit integers (that aren't lua integers)
* Use int64 in struct rather than double.
* Fix sjson to do the right things in LUA5.3 with integers and floats
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants