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

Fixes for Visual Studio. #12

Open
wants to merge 4 commits into
base: tangram
Choose a base branch
from
Open

Conversation

lygstate
Copy link

@lygstate lygstate commented Feb 9, 2017

No description provided.

@lygstate lygstate force-pushed the tangram branch 2 times, most recently from a7cff7a to e77f26a Compare February 9, 2017 02:41
@matteblair
Copy link
Member

I realized that we don't actually have an easy way to test Windows builds, so I created an Appveyor build script in another PR: #13

The Appveyor build currently fails, so it seems like these fixes are needed! Once the Appveyor script is merged, this branch should be rebased on top of it so that these changes will be built on Appveyor.

@matteblair
Copy link
Member

Merged the Appveyor script - @lygstate can you rebase onto the newest commit on the tangram branch? Thanks

@hjanetzek
Copy link
Member

hjanetzek commented Feb 9, 2017

@lygstate thanks for working on the Windows build!

For the exp matchers I think we can remove all the enable_if specializations and only leave the generic case. The compiler should be able to optimize this by itself. (Also I cannot see how your changes would implement the intended SFINAE behavior)

hjanetzek and others added 3 commits February 10, 2017 10:20
# Conflicts:
#	include/yaml-cpp/node/detail/node.h
#	include/yaml-cpp/node/impl.h
#	include/yaml-cpp/node/ptr.h
#	src/exp.h
#	src/simplekey.cpp
@lygstate
Copy link
Author

The appveyor is not valid, I'll submit a valid version later.

@matteblair
Copy link
Member

Oh I see, the Appveyor script failed to run the tests. The test runner is an executable at the path build/test/run-tests, the Appveyor script should execute that file as the test step.

@lygstate
Copy link
Author

ctest is enough to do the tests see my changes

@matteblair
Copy link
Member

You're right, ctest seems to work! Thanks.

@hjanetzek you had suggestions for the matcher templates - can you add your suggestions to the PR or create a patch to try?

@hjanetzek
Copy link
Member

@lygstate the changes you made to plalloc leak all memory. For now I would just disable the pooling with MSVC unless you want to work on it.

@lygstate
Copy link
Author

@hjanetzek Thanks, that's OK.

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