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

fix a bug of strtold #8441

Merged
merged 1 commit into from
Feb 6, 2023
Merged

fix a bug of strtold #8441

merged 1 commit into from
Feb 6, 2023

Conversation

flyingfish89
Copy link
Contributor

@flyingfish89 flyingfish89 commented Feb 6, 2023

Summary

if input is -1e308,The output should be -100000000......
but the output is -inf

See discussion here: #7992

Impact

Testing

sim:lua

  if input is -1e308,The output should be -100000000......
  but the output is -inf
@xiaoxiang781216 xiaoxiang781216 merged commit de21025 into apache:master Feb 6, 2023
@fjpanag
Copy link
Contributor

fjpanag commented Feb 6, 2023

Indeed this PR fixed the issue, and tests were able to move forward.

However, now there is a new Lua test failing.
I guess it is related, but I won't have the time to investigate it today.

Please have a look bellow:

NuttShell (NSH) NuttX-12.0.0
nsh> lua
Lua 5.4.0  Copyright (C) 1994-2020 Lua.org, PUC-Rio
> assert(0e12 == 0 and .0 == 0 and 0. == 0 and .2e2 == 20 and 2.E-1 == 0.2)
stdin:1: malformed number near '0e12'
> 

@flyingfish89
Copy link
Contributor Author

Indeed this PR fixed the issue, and tests were able to move forward.

However, now there is a new Lua test failing. I guess it is related, but I won't have the time to investigate it today.

Please have a look bellow:

NuttShell (NSH) NuttX-12.0.0
nsh> lua
Lua 5.4.0  Copyright (C) 1994-2020 Lua.org, PUC-Rio
> assert(0e12 == 0 and .0 == 0 and 0. == 0 and .2e2 == 20 and 2.E-1 == 0.2)
stdin:1: malformed number near '0e12'
> 

could you please change strtold to look like this and then test if there are any other questions
捕获

@fjpanag
Copy link
Contributor

fjpanag commented Feb 7, 2023

@flyingfish89 I tried the above change, and indeed it fixes the issue. The tests move forward.

However, I get a new failure at:

assert(tonumber'+ 0.01' == nil and tonumber'+.e1' == nil and
       tonumber'1e' == nil     and tonumber'1.0e+' == nil and
       tonumber'.' == nil)

After commenting out the failing tests, I get a new failure at:

assert(f(tonumber('  ')) == nil)

(And maybe there are more after that...)

@flyingfish89
Copy link
Contributor Author

Can you show me how to build your test program, and then I will fix the rest of the bugs as soon as possible

@flyingfish89
Copy link
Contributor Author

flyingfish89 commented Feb 7, 2023

Can you show me how to build your test program, and then I will fix the rest of the bugs as soon as possible

I ’m sorry about this. @fjpanag

@fjpanag
Copy link
Contributor

fjpanag commented Feb 7, 2023

I ’m sorry about this.

Don't worry!

So, I am using the official Lua tests suite.
Tests can be found here.

I am using Lua v5.2.4, so I am running these tests.
The problematic file is math.lua, as you guessed, which is also attached. So you can concentrate on this, instead of running all the tests.

Note that the NuttX config is for Lua v5.4, while I am running v5.2.
However, as it turns out, all issues are reproducible on the standard config, using Lua shell directly through NSH.

So, you may just try on sim:lua.

Feel free to ask for anything that you may need me to test out, or for any Lua-related help.

math.lua.txt

@fjpanag
Copy link
Contributor

fjpanag commented Feb 7, 2023

As a side note, the Lua tests are quite extensive. They test both the language itself but also the system it runs on.

Lua depends on standards compliance, and the tests exercise this in many places.

I think that it would be a great addition to have CI run these tests.
Although strictly speaking it is not NuttX testing, it has indicated me lots of OS-level problems in the past.

It seems of great value.

@flyingfish89
Copy link
Contributor Author

ok,thanks for your suggestion and I'll use it in my future work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants