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

[regression] Workaround MSVC preprocessor issue triggered by REQUIRE_THROWS #213

Merged
merged 1 commit into from Mar 26, 2019

Conversation

zhihaoy
Copy link
Contributor

@zhihaoy zhihaoy commented Mar 26, 2019

In 2.3.0 REQUIRE_THROWS goes through one extra level
when passing its expr argument, and this seems to
cause MSVC 19.16 preprocessor to refuse to do
stringification with C2026 (string too big, trailing
characters truncated). The string is indeed big (6.8KB),
but not that big (16KB).

In 2.3.0 REQUIRE_THROWS goes through one extra level
when passing its expr argument, and this seems to
cause MSVC 19.16 preprocessor to refuse to do
stringification with C2026 (string too big, trailing
characters truncated).  The string is indeed big (6.8KB),
but not that big (16KB).  I'll see if I can give minimal
reproduce to the VS team.
@onqtam
Copy link
Member

onqtam commented Mar 26, 2019

Can you paste an example of what the code looks like (with the string shortened from 6.8kb to a few chars)? This might be a problem for other macros as well.

I'll go ahead and merge this since it is a simplification anyway but you'll have to use the dev branch for now

@onqtam onqtam merged commit 61075d7 into doctest:dev Mar 26, 2019
@zhihaoy
Copy link
Contributor Author

zhihaoy commented Mar 26, 2019

Can you paste an example of what the code looks like (with the string shortened from 6.8kb to a few chars)? This might be a problem for other macros as well.

I did more research and found the cause. The code used to work because the traditionally MSVC preprocessor only expand one layer of macros in stringification each time you pass it down.

My code looks like this

REQUIRE_THROWS(somefunc(
         KWARG(.labels, "x1", "y1"),
         KWARG(.ppg, 0.3),
         ...);

Only 5 lines, but each KWARG expands to a complicated PEGTL_STRING (strongly typed compile-time string) inside tuple along with some macros -- at this stage, it's 6.8K. But if all macros are expanded, it's 799K. The standard seems to require this to be fully expanded. However, it works in gcc because gcc has no string literal size limit. It worked in msvc because passing it down once doesn't cause the whole thing to be fully expanded. It does not work with msvc next-gen preprocessor because it has both the "correct" behavior and the limit (https://devblogs.microsoft.com/cppblog/msvc-preprocessor-progress-towards-conformance/). So what I should probably is to change the call to a named lambda instead.

I'll go ahead and merge this since it is a simplification anyway but you'll have to use the dev branch for now

I'm wandering whether there is a way to not to expand the nested macros at all. Because after all users only want to see the code they wrote, not the code compiler wrote, when encounters a test failure.

@zhihaoy zhihaoy deleted the fix-msvc-pp branch March 26, 2019 06:58
@zhihaoy
Copy link
Contributor Author

zhihaoy commented Mar 26, 2019

(Anyhow the change you just merged has its value [although I sent the PR without digging into the issue too deep]: without the change REQUIRE_THROWS's test failure message is different from that of REQUIRE_THROWS_AS under msvc preprocessor's "friendly" behavior.)

onqtam pushed a commit that referenced this pull request May 6, 2019
In 2.3.0 REQUIRE_THROWS goes through one extra level
when passing its expr argument, and this seems to
cause MSVC 19.16 preprocessor to refuse to do
stringification with C2026 (string too big, trailing
characters truncated).  The string is indeed big (6.8KB),
but not that big (16KB).  I'll see if I can give minimal
reproduce to the VS team.
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.

None yet

2 participants