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

Separation of public and private header files #12

Closed
db-tech opened this issue May 7, 2021 · 12 comments
Closed

Separation of public and private header files #12

db-tech opened this issue May 7, 2021 · 12 comments
Labels
feature New feature

Comments

@db-tech
Copy link

db-tech commented May 7, 2021

wouldn't it be nice to separate header files needed externally and those only needed internally?
Maybe put all internal cpp and header files needed only internally together into the src directory and the public header into include/uvgRTP ?

@altonen
Copy link
Collaborator

altonen commented May 13, 2021

Hi,

yes it would be nicer but I personally don't see it as a huge issue that they are all in the same directory. If you want this to be changed, please open a pull request.

@db-tech
Copy link
Author

db-tech commented May 14, 2021

If you consider accepting this style, I will create a PR

@jrsnen
Copy link
Member

jrsnen commented May 17, 2021

I would also consider this an improvement. Thank you for the suggestion.

@jrsnen
Copy link
Member

jrsnen commented Jun 1, 2021

What is the situation on the PR? I could attempt to do this. It seems there are some header dependencies that would probably need to be eliminated.

@db-tech
Copy link
Author

db-tech commented Jun 1, 2021

Wanted to start with that next week. But if you have the time, be my guest :-)
Then I would do issue #13.
That kind of depends on this one.
If you start with it and need some input or help, let me know.

@jrsnen
Copy link
Member

jrsnen commented Jun 2, 2021

I'm sorry, but this proved to be too big of a task for me at this point. I managed to reduce header dependencies, but there is no clear API defined in uvgRTP, which makes it hard to determine which headers are necessary and which are not. In addition, I was not able to remove all the dependencies (in rtcp.hh) which makes it difficult to eliminate those headers. I'll update my changes so far this week.

@altonen Do you have a list of which headers are necessary to use the library and which are meant to be internal headers? For example, I sow the need to free the frame using function from frame.hh which I thought was internal.

@jrsnen
Copy link
Member

jrsnen commented Jun 2, 2021

I updated the repo with reduced header dependencies.

lib.hh now includes all the header files since I didn't know which ones were required. This is basically the same as before because of the header dependencies.

@jrsnen
Copy link
Member

jrsnen commented Jun 4, 2021

After a discussion with @altonen, we settled on a list of headers that are needed in the include folder. There are few extra because of rtcp.hh, but in general decde85 is a big improvement on this matter. Feel free to improve it further if you have ideas.

@db-tech
Copy link
Author

db-tech commented Jun 4, 2021

Awesome, i will take a Look tomorrow.

@db-tech
Copy link
Author

db-tech commented Jun 5, 2021

I did a minimalistic check of the commit. I would need more time to check if we could do more Improvements. Unfortunately, I am not able to be more thorough at the moment.

I still have some comments:

  1. I would consider creating a subdirectory uvgrtp in the include directory. That would be nicer in the internal include statements and easy to distinguish between private and public header.
  2. I would try to avoid relative include paths like this "../xyz.hh" but rather adding the src dir to the private include paths in cmake.

@jrsnen
Copy link
Member

jrsnen commented Jun 9, 2021

I create a separate issue for your comment 2. Comment 1 should be implemented and is related to this issue.

@jrsnen
Copy link
Member

jrsnen commented Jun 16, 2021

Merged with #44

@jrsnen jrsnen closed this as completed Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

No branches or pull requests

3 participants