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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move constructor support for Writer #740

Merged
merged 3 commits into from
Sep 20, 2016

Conversation

CreoValis
Copy link
Contributor

As internal::Stack already has a move constructor, it's trivial to write one for Writer.
(Plus, i needed it for a project of mine 馃槃 .)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 99.935% when pulling 49540ac on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage remained the same at 99.937% when pulling 21a6676 on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

@miloyip
Copy link
Collaborator

miloyip commented Sep 19, 2016

Please add unit tests for code coverage.
Please try to imitate the coding style.
And also, should PrettyWriter need the same thing?
Thank you.

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage remained the same at 99.937% when pulling 2b73169 on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

@CreoValis
Copy link
Contributor Author

Thanks!
I've improved on the code style, and added move ctor support for PrettyWriter. Will add tests soon.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 99.935% when pulling 878eef8 on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

@CreoValis CreoValis force-pushed the writer-move-ctor branch 2 times, most recently from c915e91 to a5e6ba2 Compare September 19, 2016 18:14
@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage remained the same at 99.937% when pulling a5e6ba2 on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage remained the same at 99.937% when pulling a5e6ba2 on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

This also requires turning off the c++98 compatibility checks when building
with clang.
@CreoValis CreoValis force-pushed the writer-move-ctor branch 2 times, most recently from 3b2156d to cf18d90 Compare September 19, 2016 20:05
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 99.935% when pulling cf18d90 on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage increased (+8.0e-05%) to 99.937% when pulling 1a64cd0 on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

@coveralls
Copy link

coveralls commented Sep 19, 2016

Coverage Status

Coverage increased (+8.0e-05%) to 99.937% when pulling 1a64cd0 on CreoValis:writer-move-ctor into 5268211 on miloyip:master.

@CreoValis
Copy link
Contributor Author

I've added unit tests for both classes' move constructors, and both CI tests pass now.
Is the code style acceptable in it's current form?

@miloyip miloyip merged commit 185a7cc into Tencent:master Sep 20, 2016
@miloyip
Copy link
Collaborator

miloyip commented Sep 20, 2016

Thank you

@CreoValis CreoValis deleted the writer-move-ctor branch September 20, 2016 12:32
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.

None yet

3 participants