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 dmlc::strtof() #481

Merged
merged 11 commits into from
Nov 12, 2018
Merged

Fix dmlc::strtof() #481

merged 11 commits into from
Nov 12, 2018

Conversation

hcho3
Copy link
Contributor

@hcho3 hcho3 commented Nov 10, 2018

Previous implementation of locale-independent parsing (#455) was faulty:

  1. It broke builds for MinGW-w64 + Windows 7 target (Fixes recently introduced error with _create_locale and msvcrt #478)
  2. It doesn't parse 0 correctly (Locale-agnostic parameter parsing #455 (comment)).
  3. The use of locale functions adds too much complexity, which goes against the spirit of dmlc-core as basic building blocks.

This PR rectifies the situation by re-using existing implementation of strtof(). In particular,

  1. dmlc::data::strtof() is moved to include/ directory.
  2. Add template parameters to the strtof() implementation to add range checking and to implement dmlc::strtod().
  3. Add a better test for float parameter parsing.

Also, the locale functions have been removed.

Closes #478.

@hcho3
Copy link
Contributor Author

hcho3 commented Nov 10, 2018

@trivialfis I've removed platform-dependent code for the sake of future maintenance burden.

@hcho3
Copy link
Contributor Author

hcho3 commented Nov 10, 2018

TODO. Add docstrings for all public functions in include/dmlc/strtonum.h.

@KOLANICH
Copy link
Contributor

Works fine, thank you.

@hcho3
Copy link
Contributor Author

hcho3 commented Nov 12, 2018

@KOLANICH Glad it solved your problem.

TODO: use libc-test to rigorously test dmlc::strtof() and dmlc::strtod().

@hcho3
Copy link
Contributor Author

hcho3 commented Nov 12, 2018

It turns out dmlc::strtof() won't be C standard compliant, since it cannot parse INF/NAN and cannot correctly handle long decimals. Will need to clearly document limitations.

@hcho3 hcho3 merged commit 831089a into dmlc:master Nov 12, 2018
@hcho3 hcho3 deleted the fix_strtof branch November 12, 2018 22:46
ruslo pushed a commit to hunter-packages/dmlc-core that referenced this pull request Mar 23, 2019
* Move strtonum.h to include/

* Implement range checking into dmlc::strtof(); add dmlc::strtod()

* Fix build

* More example of locale-independent parsing

* Add docstrings; rename functions

* Implement stof(), stod(); add extra check to FieldEntry<float>, FieldEntry<double>

* Fix lint

* Clearly document limitations

* Make types explicit

* Throw error when given too many digits before the decimal point

* handle edge case where exponent is exactly kMaxExponent
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.

2 participants