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

tool_paramhelp: str2udouble 'max' changed to double #9700

Closed
wants to merge 1 commit into from

Conversation

Ricardicus
Copy link
Contributor

It is Hacktoberfest. I found myself here. Nitpicking in this glorious repository.

My LSP told me:

"Implicit conversion from 'long' to 'double' may lose precision"

I therefore thought that I perhaps could make a little PR.

Actually, with this PR; one may now be able to wait even longer for a connection to fail!
And that might also be a good thing.

@bagder
Copy link
Member

bagder commented Oct 12, 2022

The maximum accepted timeout value for connect timeout is set to be LONG_MAX/1000 for a reason:

  1. CURLOPT_CONNECTTIMEOUT_MS accepts a long
  2. The option takes a millisecond counter so we multiply the given value by 1000
  3. We therefore need the value to be be less than LONG_MAX/1000 to not overflow/wrap in the math

@Ricardicus
Copy link
Contributor Author

Ok, I see. I will change it back to LONG_MAX/1000

@Ricardicus
Copy link
Contributor Author

Maybe it should be long all the way?

@bagder
Copy link
Member

bagder commented Oct 12, 2022

Maybe it should be long all the way?

It parses a double to allow fractions of seconds: 1.234, but it could certainly store that as milliseconds in a long.

@Ricardicus
Copy link
Contributor Author

I see.. I will think of a way to perhaps not compare a long to a double; that would be the only value in a PR like this hehe.

@Ricardicus
Copy link
Contributor Author

I think this is as far as this PR can go!

@bagder
Copy link
Member

bagder commented Oct 13, 2022

"This branch cannot be rebased due to conflicts"

Can you please rebase on master, fix the issues and force-push?

@Ricardicus
Copy link
Contributor Author

There it is!

@bagder
Copy link
Member

bagder commented Oct 15, 2022

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants