Skip to content

Conversation

Zenju
Copy link

@Zenju Zenju commented Oct 13, 2019

Hey STL,

just watched your cppcon talk. I couldn't help myself and had to implement std::to_chars/std::from_chars immediately. Had a few issues:

  1. #include <charconv> in my precompiled header file:
    => compiler error: "fatal error C1076: compiler limit: internal heap limit reached"

  2. #include <charconv> in another header, widely-used in my project:
    => compiler error: "An internal error has occurred in the compiler. (compiler file 'd:\agent_work\1\s\src\vctools\Compiler\Utc\src\p2\p2symtab.c', line 2618)"

=> created a wrapper and #include <charconv> locally in a .cpp file => fine.

  1. Runtime check error in std::to_chars (for double(12.34))
    => patch included

Best, Zenju
fail

@Zenju Zenju requested a review from a team as a code owner October 13, 2019 11:56
@sylveon
Copy link
Contributor

sylveon commented Oct 13, 2019

Pretty sure this is not a supported scenario:

T:\Projects>type test.cpp
#include <iostream>

T:\Projects>cl /RTCc test.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.22.27905 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

test.cpp
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.22.27905\include\yvals_core.h(840): fatal error C1189: #error:  /RTCc rejects conformant code, so it is not supported by the C++ Standard Library. Either remove this compiler option, or define _ALLOW_RTCc_IN_STL to acknowledge that you have received this warning.

@BillyONeal
Copy link
Member

BillyONeal commented Oct 13, 2019

(1) suggests that we might need to move the ryu tables to be in the import lib. Can you file that as an issue?
(2) is a compiler bug, not an STL bug. Please report through https://developercommunity.visualstudio.com/spaces/62/index.html with a self contained repro.
(3) is /RTCc, which is, as @sylveon indicated, not supported.

I defer to @StephanTLavavej on anything <charconv> related, but given that we don't support /RTCc I don't think we should merge this.

@StephanTLavavej
Copy link
Member

Thanks for trying <charconv>! I've filed #172 to track moving the lookup tables into the static/import lib. (As a workaround, you can use /Zm to increase the PCH size; this should be necessary for x86 only, as x64 uses a massively larger default.)

We blocked /RTCc for exactly this reason - it rejects perfectly conformant code, and there are other places in the STL that are incompatible with it. I agree with @BillyONeal that this change shouldn't be merged, so I'm closing this PR.

If you're curious as to what's happening here: we are truncating uint64_t to uint32_t because we have a mathematical guarantee that we don't need the upper bits. We're starting with uint64_t vr and calculating vr % 100 as vr - 100 * (vr / 100). (This benefits earlier versions of Clang which had difficulty seeing the relationship between modulo and division.) To benefit 32-bit platforms, we don't actually need to use 64-bit math for this. By definition, vr % 100 is within [0, 99]. That means that in the subtraction, vr and 100 * (vr / 100) must match in all of their high bits (only the lowest 7 can differ because 27 - 1 = 128). Therefore we can simply discard the upper 32 bits and perform the math in uint32_t.

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.

4 participants