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.

Sign up for GitHub

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

Jump to bottom

util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) #2564

Merged
merged 1 commit into from
Oct 7, 2022
Merged

util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) #2564

merged 1 commit into from
Oct 7, 2022

Conversation

barton2526
Copy link
Member

Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17).

Ref: bitcoin/bitcoin#20452

@barton2526
Copy link
Member Author

@div72 This is failing in the Bionic CI. It is unable to locate charconv, which is a C++17 standard library. Our documentation claims we require a C++17 compiler (see assumptions file and configure) yet somehow our own CI runner doesn't support the standard? What is going on here?

@div72
Copy link
Member

div72 commented Aug 9, 2022 •
edited
Loading

GCC 7 supports C++17 mostly, charconv header is added in GCC 8 11 which requires a bump to impish AFAICT. See P0067R5 in https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2017

@Pythonix
Copy link
Contributor

Pythonix commented Aug 9, 2022

The issue has been discussed upstream bitcoin/bitcoin#20452 (comment) and I think the following PR resolved the issues bitcoin/bitcoin#23060.

@div72
Copy link
Member

div72 commented Aug 9, 2022

@Pythonix The fix for the issue is to use a newer GCC. We generally wait for the EOL before doing bumps like these.

@Pythonix
Copy link
Contributor

Pythonix commented Aug 9, 2022

@Pythonix The fix for the issue is to use a newer GCC. We generally wait for the EOL before doing bumps like these.

I know, that is what is done in bitcoin/bitcoin#23060. GCC is bumped to 8.1.

@barton2526
Copy link
Member Author

barton2526 commented Aug 9, 2022 •
edited
Loading

@Pythonix The fix for the issue is to use a newer GCC. We generally wait for the EOL before doing bumps like these.

The trouble here is that Bionic doesn't go EOL until April 2023. Do we really have to be stuck without C++17 features (even though our documentation states we are using C++17) for over half a year?

@barton2526
Copy link
Member Author

barton2526 commented Aug 9, 2022 •
edited
Loading

I'd also like to point out gcc-8 is available on Bionic, just not the default (https://packages.ubuntu.com/search?suite=bionic&keywords=gcc-8). Can't we simply change the CI runner to install gcc-8 and change our build docs in the interim?

@div72
Copy link
Member

div72 commented Aug 12, 2022

I'd also like to point out gcc-8 is available on Bionic, just not the default (https://packages.ubuntu.com/search?suite=bionic&keywords=gcc-8). Can't we simply change the CI runner to install gcc-8 and change our build docs in the interim?

I seem to have misread the docs, yeah that should be fine.

@barton2526
Copy link
Member Author

@jamescowens is the above fine with you as well? If so, I'll add a PR tomorrow to bump the CI runner and docs to require gcc-8.

@jamescowens
Copy link
Member

I know that up version compilers are available on other distributions as well. The CI and packaging may have different pressure points on this. Is this going to cause a problem for maintainers? I would like @caraka's take on this first.

@jamescowens jamescowens added compatibility deprecation Removed deprecated functionality labels Aug 16, 2022
@caraka
Copy link
Member

caraka commented Sep 13, 2022

I will do some homework, I'm pretty sure I can bump the compiler at Launchpad.

@barton2526
Copy link
Member Author

barton2526 commented Oct 4, 2022 •
edited
Loading

I will do some homework, I'm pretty sure I can bump the compiler at Launchpad.

Any update?

@jamescowens
Copy link
Member

jamescowens commented Oct 4, 2022 via email

@barton2526 barton2526 mentioned this pull request Oct 5, 2022
Merged
@jamescowens
Copy link
Member

@barton2526 can you rebase this on the head of development now that I merged #2579?

@jamescowens jamescowens added this to the LaVerne milestone Oct 6, 2022
@barton2526
Copy link
Member Author

Rebased

jamescowens
jamescowens approved these changes Oct 7, 2022
View reviewed changes
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

ACK

@jamescowens jamescowens merged commit bcb3b53 into gridcoin-community:development Oct 7, 2022
@div72 div72 removed their request for review October 7, 2022 19:26
@barton2526 barton2526 deleted the remove-atoi branch October 12, 2022 02:12
jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Nov 27, 2022
@jamescowens
5.4.1.0 2022-11-27, leisure
6a4285a 
Added
 - net: Add and document network messages in protocol.h (backport) gridcoin-community#2533 (@Pythonix)
 - Define MAX_DIGITS_BTC for magic number in BitcoinUnits::format gridcoin-community#2555 (@barton2526)
 - rpc: Implementation of getmrcinfo gridcoin-community#2570 (@jamescowens)
 - init: Add init error message if -printtoconsole and -daemon specified simultaneously gridcoin-community#2571 (@jamescowens)
 - rpc: getmrcinfo part 2 - add calculated minimum fees and fee boosting and by CPID reporting gridcoin-community#2575 (@jamescowens)
 - fs: fully initialize `_OVERLAPPED` for win32 gridcoin-community#2587 (@div72)
 - util: Diagnose Lib Version #1 gridcoin-community#2573 (@MinaFarhan)
 - util: Implement core diagnostics #2 (@jamescowens)
 - util: modify Win32LockedPageAllocator to query windows for limit. gridcoin-community#2536 (@div72)
 - gui, voting: Implement information for wallet holder's votes on poll info cards gridcoin-community#2605 (@jamescowens)

Changed
 - scripted-diff: Drop Darwin version for better maintainability gridcoin-community#2557 (@barton2526)
 - build: Require gcc8 on Ubuntu Bionic to enable C++17 features gridcoin-community#2579 (@barton2526)
 - util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) gridcoin-community#2564 (@barton2526)
 - translation: Translation updates gridcoin-community#2581 (@jamescowens)
 - depends: update urls for dmg tools gridcoin-community#2583 (@div72)
 - Use ReadLE64 in uint256::GetUint64 instead of duplicating logic gridcoin-community#2586 (@div72)
 - util: Make Parse{Int,UInt}{32,64} use locale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull} gridcoin-community#2592 (@barton2526)
 - build: don't set PORT=no in config.site gridcoin-community#2593 (@barton2526)
 - build: Replace `which` command with `command -v` gridcoin-community#2595 (@barton2526)
 - build: update ax_cxx_compile_stdcxx to serial 14 gridcoin-community#2596 (@barton2526)
 - gui: Changed the unlocked for staking only icons to green gridcoin-community#2598 (@delta1513)
 - gui: Translation updates gridcoin-community#2599 (@jamescowens)
 - build: update CI for linter and actions version gridcoin-community#2606 (@jamescowens)
 - gui: Update translations gridcoin-community#2608 (@jamescowens)

Removed
 - refactor: remove unused c-string variant of atoi64() gridcoin-community#2562 (@barton2526)
 - refactor: Remove unused CDataStream::rdbuf method gridcoin-community#2585 (@div72)

Fixed
 - net: Fix some benign races (backport) gridcoin-community#2532 (@Pythonix)
 - rpc: fix invalid parameter error codes for {sign,verify}message RPCs gridcoin-community#2556 (@barton2526)
 - build: Fix x86_64 <-> arm64 cross-compiling for macOS gridcoin-community#2560 (@barton2526)
 - rpc, mrc: Fix field name and initialization of mrc_fees_to_staker gridcoin-community#2567 (@jamescowens)
 - gui: Add missing resizeTableColumns to fix send address book column widths gridcoin-community#2569 (@jamescowens)
 - accrual: rebuild snapshot registry on corruption instead of crashing gridcoin-community#2577 (@div72)
 - doc: Fix link to MurmurHash3.cpp (moved from Google Code to Github) gridcoin-community#2584 (@div72)
 - fix help text for `revokebeacon` command gridcoin-community#2591 (@Pythonix)
 - util: Fix spelling error in gridcoinresearchd.cpp gridcoin-community#2590 (@jamescowens)
 - depends: always use correct ar for win qt build gridcoin-community#2588 (@div72)
 - util: Fix some bugs due to new implementation and change in BOINC dir handling (@jamescowens)
 - util: Diagnose lib - Implement changes to solve crash on some Boost 1.66 machines gridcoin-community#2597 (@jamescowens)
 - contrib: Check for `patch` command, Check for `wget` command gridcoin-community#2594 (@barton2526)
 - build: Check std::system for -[alert|block|wallet]notify gridcoin-community#2582 (@barton2526)
 - gui: Changed the wording on the tooltip for the address book gridcoin-community#2602 (@delta1513)
 - build: pass win32-dll to LT_INIT() gridcoin-community#2601 (@barton2526)
 - build: minor cleanups to native_clang package gridcoin-community#2600 (@barton2526)
 - util: restore translations to diagnostics gridcoin-community#2603 (@jamescowens)
 - refactor: Fix problems found by valgrind gridcoin-community#2607 (@jamescowens)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamescowens jamescowens jamescowens approved these changes

Assignees

@barton2526 barton2526

Labels
compatibility deprecation Removed deprecated functionality
Projects
None yet
Milestone
LaVerne
Development

Successfully merging this pull request may close these issues.
5 participants
@barton2526 @div72 @Pythonix @jamescowens @caraka