Simplify INI-style config reader using C++11 STL #4478
Conversation
|
@fuhaoda Hi, any update on this? And could you please add some simple tests? |
Is it only a format issue? I don't know what's wrong here. It can be compiled and pass test on my Mac, and I compared the results and test case. All are the same before I submit the pull request. |
|
@fuhaoda It has some linter issues on Linux, and somehow failed tests on Windows. Would you like to fix the linter first so that we can see how's it doing on Linux? See: https://xgboost-ci.net/blue/organizations/jenkins/xgboost/detail/PR-4478/1/pipeline and look for some lines like this:
|
|
@trivialfis Thank you so much! Trying really hard and fixed the linter issues. Could you please take a look on what's the issue on Windows test? My guess is the path issue to include character like . |
|
@fuhaoda You were using older versions of the submodules |
hcho3
changed the title
Co-Authored-By: Philip Hyunsu Cho <chohyu01@cs.washington.edu>
|
@hcho3 any thoughts on the check failures? it was successful yesterday... |
|
@fuhaoda Looks like it's out of memory. Let me re-start the test again. Fundamentally, the tests tend to consume a lot of memory, so we often get out-of-memory errors. I'm trying to find ways to reduce memory consumption in tests. |
src/common/config.h
Outdated
| std::istream &fin_; | ||
| }; | ||
| std::ifstream fi_; | ||
| std::string allowableChar = |
There was a problem hiding this comment.
I'm curious why clang-tidy didn't catch this styling error.
There was a problem hiding this comment.
@trivialfis I don't see any issue with this line?
There was a problem hiding this comment.
@hcho3 private member suffix '_'. I tried clang-tidy-8 and it didn't catch it either.
src/common/config.h
Outdated
| @@ -1,8 +1,8 @@ | |||
| /*! | |||
| * Copyright 2014 by Contributors | |||
| * Copyright 2019 by Contributors | |||
|
@fuhaoda Sorry for the late reply. I can't push to your branch, so could you apply the following patch and I will merge? |
|
@trivialfis Thanks a lot. Just followed your suggestion and changed them all on the format issues based on the patch that you sent to me. @hcho3 Thanks for the suggestions as well! |
|
@hcho3 is it an out of memory issue again? could you please re-start the test again? looking forward to the final merge. |
|
@fuhaoda No, this time it was compilation failure, because the |
|
@hcho3 likely to need your help again : ) failed in a different test |
Codecov Report
@@ Coverage Diff @@
## master #4478 +/- ##
=======================================
Coverage 79.42% 79.42%
=======================================
Files 10 10
Lines 1735 1735
=======================================
Hits 1378 1378
Misses 357 357
Continue to review full report at Codecov.
|
|
@hcho3 I will leave it for you to merge it. Thanks for all the helps here. |
|
@trivialfis I'm still looking for ways to eliminate out-of-memory errors that pop up once in a while. |
This reverts commit dd60fc2.
- This reverts commit dd60fc2. - create a row state per device that keeps track of spans of the sparse pages that needs to be processed
Use C++11 STL to simplify the "./src/common/config.h" file.