8000 Feature msvc clang by GabrielHare · Pull Request #141 · unittest-cpp/unittest-cpp · GitHub
[go: up one dir, main page]

Skip to content

Feature msvc clang #141

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

Merged
merged 7 commits into from
Jul 1, 2017
Merged

Conversation

GabrielHare
Copy link
Contributor
@GabrielHare GabrielHare commented Dec 15, 2016

UnitTest++ supports clang as a compiler for Visual Studio via native extension

For information on enabling & using the extension see:
https://blogs.msdn.microsoft.com/vcblog/2015/12/04/clang-with-microsoft-codegen-in-vs-2015-update-1/
(Since that post, an update offers v140_clang_c2)
And, for instruction for CMake see:
https://github.com/boostorg/hana/wiki/Setting-up-Clang-on-Windows

However, the CMakeLists.txt for UnitTest++ uses the MSVC variable to test for the compiler type, which will be incorrect when clang is used.

This PR uses a test against the compiler name instead. It has been verified with:

  • cmake 3.7.0 (Windows)
  • clang 3.8.0 (v140_clang_c2 Visual Studio 2015)
  • msvc 19.0.24215.1 (Visual Studio 2015)
  • cmake 3.5.2 (Ubuntu)
  • GCC 6.2.0 (Ubuntu)
  • clang 3.8.1 (Ubuntu)
  • cmake 3.4.3 (Apple)
  • AppleClang 8.0.0 generated for XCode
  • AppleClang 8.0.0 generated for command line make

NOTE: It is also possible to attempt to install and use LLVM & Clang, but the compiler flags are a mess
http://releases.llvm.org/download.html
http://clang.llvm.org/docs/MSVCCompatibility.html

@grahamreeds
Copy link
Contributor
grahamreeds commented Dec 15, 2016 via email

@GabrielHare
Copy link
Contributor Author

@grahamreeds Makes sense, and I see the error in Travis - I'll fix & update.

…g implicit quoted dereference in STREQUAL comparison"

This reverts commit 8f7a263.
NOTE: Unlike STREQUAL this comparison is supported pre and post CMake v3.1.0
@GabrielHare
Copy link
Contributor Author

@grahamreeds update passes tests now - I hope you'll support this PR

@pjohnmeyer
Copy link
Member

Your changes made me notice the portion of the CMake message that reads "Please use a different C++ compiler." That message is misleading; compilers that don't support C++11 still work with UnitTest++.

As such I don't consider this a warning; please change the new and existing message to STATUS level, and while you're at it I'd be pleased if you removed that sentence.

Copy link
Member
@pjohnmeyer pjohnmeyer left a comment

Choose a reason for hiding this comment

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

See prior comment.

@GabrielHare
Copy link
Contributor Author
GabrielHare commented Jan 20, 2017

Thank you for taking a look at this.
Your requested changes appear to elevate the message level from STATUS to WARNING and do not remove the statement that the a different compiler should be used.
I'm happy to make your requested changes (as I understand them) or accept the changes - I'm just unsure to do since they seem to me to be contradictory requests.

(Also, please note that this PR removes the building of TestUnitTest++ from "all" target.)

@grahamreeds
Copy link
Contributor
grahamreeds commented Jan 20, 2017 via email

@GabrielHare
Copy link
Contributor Author
GabrielHare commented Jan 20, 2017

Actually, that's not in this PR - I can open another with that (and CI changes).

Regarding why: including tests in all makes development simpler, but makes rebuilding a project that uses UnitTest++ take longer.

@grahamreeds
Copy link
Contributor
grahamreeds commented Jan 20, 2017 via email

@GabrielHare
Copy link
Contributor Author

In my case I'm using UnitTest++ in my project via CMake's ExternalProject_Add so for each complete rebuild of the project I'm also rebuilding UnitTest++, and would be rebuilding TestUnitTest++ if it were not excluded.

I'm guessing that I'm not unusual in building UnitTest++ when building a project.

@pjohnmeyer
Copy link
Member

Actually, that's not in this PR - I can open another with that (and CI changes).

You can already skip building and running TestUnitTest++ by setting the CMake build option UTPP_INCLUDE_TESTS_IN_BUILD to OFF, so the secondary pull request won't be necessary.

@GabrielHare
Copy link
Contributor Author

@pjohnmeyer thank you for the cmake flag hint.

Copy link
Contributor Author
@GabrielHare GabrielHare left a comment

Choose a reason for hiding this comment

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

Requested changes made in commit updating this PR

@GabrielHare
Copy link
Contributor Author

@pjohnmeyer do the changes made in 0b15459 work for you?

Copy link
Member
@pjohnmeyer pjohnmeyer left a comment

Choose a reason for hiding this comment

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

These LGTM now; I resolved merge conflicts and will wait for tests to pass.

@pjohnmeyer pjohnmeyer merged commit 8c460c4 into unittest-cpp:master Jul 1, 2017
@GabrielHare
Copy link
Contributor Author

Just got the notification - thank you :-)

@pjohnmeyer pjohnmeyer modified the milestone: 2.0.1 Jul 8, 2017
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
0