search

ReactiveX / RxCpp

Reactive Extensions for C++
Apache License 2.0
3.07k stars 396 forks source link

Fails build on VS2017 RC due to Catch issue #352

Closed aargor closed 7 years ago

aargor commented 7 years ago

Hello,

I just downloaded the latest VS2017 RC and confirmed that there are build errors (from warnings) due to Catch. Essentially improvements to the libraries helped reveal a few narrowing conversions. The link https://github.com/philsquared/Catch/pull/728/files has more details. I see that the version of Catch in this project is 1.2.1. I'm not sure which Catch version has this fix, but it's later.

I'm on the VC++ team at Microsoft, and we found this issue because we use RxCPP as part of our big "real-world-code" rolling test suite.

From my own testing either cherry-picking those patches, or updating to the latest Catch version addresses this build issue. I'm not sure which you'd prefer, so I'm hesitant to make my own pull request for this.

Thanks, Aaron Gorenstein (aargor)

gchudnov commented 7 years ago

Hi aargor, thank you for reporting an issue!

It should be OK to update Catch to the latest version. Just made a PR.

Guess we'll need to add VS2017 RC to the list of compilers to test on.

Thank you, Grigoriy

aargor commented 7 years ago

Thank you for the guidance! This is my first pull request :).

I just filed a PR (I believe it's linked) and I will keep tabs on this and the PR thread if there's anything else I need to do or should tell you.

Thanks again, Aaron

aargor commented 7 years ago

A few things: 1) I completely misread "made" with "make", sorry for the redundant pull request. 2) Testing against the latest RC in the future would be great. I mentioned it in particular more to indicate that we'd expect this same issue to manifest for the official release as well. 3) Whoops, looks like something's going wrong for VS2013. I will follow up with some colleagues.

kirkshoop commented 7 years ago

This has been resolved

Thank you!