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

Remove csrc from the RTPHeader struct. #750

Merged
merged 1 commit into from
Feb 8, 2018
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Feb 2, 2018

This is not used by anything in the code, so we shouldn't have it in the
header.


This change is Reviewable

@iphydf iphydf added this to the v0.2.0 milestone Feb 2, 2018
@iphydf iphydf changed the title Remove csrc from the RTPHeader struct. #1: Remove csrc from the RTPHeader struct. Feb 2, 2018
@iphydf iphydf requested a review from a team February 2, 2018 16:23
@Diadlo
Copy link

Diadlo commented Feb 2, 2018

Why not make gtest an optional dependency and make this test an optional in cmake?

@sudden6
Copy link

sudden6 commented Feb 2, 2018

removal :lgtm_strong: don't know about the tests


Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Feb 2, 2018

Reviewed 2 of 7 files at r1.
Review status: 2 of 7 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Feb 2, 2018

Added gtest to cmake. Not running it on travis.

@nurupo
Copy link
Member

nurupo commented Feb 2, 2018

Could we not make C++ compiler required for building toxcore without having a very good reason for this? :\

@iphydf
Copy link
Member Author

iphydf commented Feb 2, 2018

It's not required for compiling it, but maybe for compiling its tests.

@iphydf
Copy link
Member Author

iphydf commented Feb 2, 2018

(some of the tests).

@nurupo
Copy link
Member

nurupo commented Feb 2, 2018

It does make sense for the monolith test, but not much for others. Why is rtp_test.cpp in C++? Why do we use gtest when we use libcheck in all other tests? This looks very out of place in toxcore.

@iphydf
Copy link
Member Author

iphydf commented Feb 2, 2018

Why C++ and gtest: it's much easier to write unit tests with it. libcheck is not much more than a glorified printf debugging library. Comparing values, printing them, etc. is spartanic. I'm not going to write more tests with libcheck, I'd rather get rid of it altogether.

Given the opposition, I've removed it from cmake again.

@iphydf iphydf force-pushed the rtp-struct branch 9 times, most recently from 4b0c356 to 84410f2 Compare February 5, 2018 18:15
@iphydf
Copy link
Member Author

iphydf commented Feb 5, 2018

gtest is back in #757, and libcheck is gone in #766.

This is not used by anything in the code, so we shouldn't have it in the
header.
@iphydf iphydf changed the title #1: Remove csrc from the RTPHeader struct. Remove csrc from the RTPHeader struct. Feb 8, 2018
@iphydf iphydf merged commit 36ba80a into TokTok:master Feb 8, 2018
@iphydf iphydf deleted the rtp-struct branch February 8, 2018 14:52
This pull request was closed.
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