Skip to content

Conversation

@srnwk
Copy link
Contributor

@srnwk srnwk commented Mar 10, 2022

On recent MSVC versions, the thrift compiler fails to build under C++17 or C++20 -- as far as I can tell this is a bug in MSVC.
Splitting that clever "read the whole file into a string"-line into several seems to avoid it, with no functional change.

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@Jens-G
Copy link
Member

Jens-G commented Mar 10, 2022

I use it with VS2022 and had not experienced any issues.

Could you enlighten us what the problem is exactly? "fails fails to build" is a bit too vague (and grammatically questionable :-))

@srnwk
Copy link
Contributor Author

srnwk commented Mar 11, 2022

I use it with VS2022 and had not experienced any issues.

Could you enlighten us what the problem is exactly? "fails fails to build" is a bit too vague (and grammatically questionable :-))

Here's a minimal repro https://godbolt.org/z/b76n9oaKv

<source>(9,102): error C2064: term does not evaluate to a function taking 0 arguments
        std::string const old_file_contents(static_cast<std::ostringstream const&>(std::ostringstream() << old_file.rdbuf()).str());
                                                                                                     ^

When compiling with /std:c++20 (or C++17 with /permissive-), this blows up with the above error.
(There's no reason why it should, as far as I can tell; clang and GCC agree).

@emmenlau
Copy link
Member

Its an unlikely problem, but at the same time the fix seems relatively harmless (and makes the code even a bit more readable). I'm not opposed to merging. @Jens-G what do you think?

@Jens-G Jens-G merged commit 8ef4ed1 into apache:master Mar 11, 2022
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.

3 participants