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

Issue 1066 #1080

Merged
merged 6 commits into from
Nov 12, 2019
Merged

Issue 1066 #1080

merged 6 commits into from
Nov 12, 2019

Conversation

BillyDonahue
Copy link
Contributor

Julian I took out the macros and tried to compress the unit tests a bit.
It's just a suggestion. You can take it or leave it, really.

@cdunn2001
Copy link
Contributor

cdunn2001 commented Nov 4, 2019

TIL about "Draft" pull-requests. Anyway, this is interesting. (re: #1066 and #1075)

@JulianvDoorn
Copy link

JulianvDoorn commented Nov 8, 2019

I have no idea how these drafts work, am I meant to merge this or is this simply an addition for #1075 ? Also- the travis-ci build fails which prevents merging, I can't seem to reproduce it on my own machine. And the error does not clearly indicate what's wrong.

@dota17
Copy link
Member

dota17 commented Nov 9, 2019

I think this PR is better.

From the ci report, it fails in the case Linux xenial gcc cmake coverage.
The Travis-CI tells that there are some errors when building and testing with CMake.
I try to reproduce it on my own machine. When i build with CMake, i got these following errors:

/src/lib_json/../../include/json/value.h:419:13: error: explicit specialization in non-namespace scope 'class Json::Value'
   template <> bool as<bool>() const { return asBool(); }
             ^
....

And i got some same build errors when building with meson/ninja.

@BillyDonahue
Copy link
Contributor Author

Oh, I forgot you can't specialize a template inside a class.
You just have to move the specializations outside the class.
I did that in 826228e

@BillyDonahue BillyDonahue marked this pull request as ready for review November 11, 2019 02:00
@BillyDonahue BillyDonahue merged commit 53c8e2c into open-source-parsers:master Nov 12, 2019
@BillyDonahue BillyDonahue deleted the issue_1066 branch November 12, 2019 03:44
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.

4 participants