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

Build Fix. #1918

Merged
merged 1 commit into from Apr 8, 2021
Merged

Build Fix. #1918

merged 1 commit into from Apr 8, 2021

Conversation

jlsantiago0
Copy link
Contributor

@jlsantiago0 jlsantiago0 commented Apr 1, 2021

bool ParseFilterConfig() 3 parameter version is called in srtcore/socketconfig.h:1072, but there is no prototype defined for it. This causes build failures with toolchains that do not auto-create an int ParseFilterConfig() for calls to undefined functions. This could also cause ABI issues if sizeof(bool) != sizeof(int) causing potential memory access violations.

@ethouris
Copy link
Collaborator

ethouris commented Apr 6, 2021

BTW. Might be a good idea to mention what exactly toolchains fail to compile this. It's because someone in future might simply remove this definition as being repeated after the friend definition of the same function inside the class and cause the build failure again.

@maxsharabayko maxsharabayko added this to the v1.4.3 milestone Apr 6, 2021
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels Apr 6, 2021
@jlsantiago0
Copy link
Contributor Author

jlsantiago0 commented Apr 6, 2021

BTW. Might be a good idea to mention what exactly toolchains fail to compile this. It's because someone in future might simply remove this definition as being repeated after the friend definition of the same function inside the class and cause the build failure again.

The makitoX toolchain for 1. And a couple of others that I was building for.

@maxsharabayko
Copy link
Collaborator

I would say some refactoring is better to be done here. 🤔
I am not addressing this to you, @jlsantiago0. Just saving my thoughts for later to see if some time could be spent here or we should just proceed with this fix and put the rest to the technical debt.

A fair comment by @quink-black that this code should better be placed under srt namespace is also worth addressing. Issue #1924 generalizes this point.

Possible Refactoring

One way I see is to change the ParseFilterConfig function to return a pointer to void instead of bool. A NULL returned would mean false, otherwise, a pointer to an existing PacketFilter::Factory is returned.

void* ParseFilterConfig(std::string s, SrtFilterConfig& w_config);

Way number two: move the abstract class PacketFilter::Factory into packet_filter_api.h. Of course, place everything under the srt namespace.
Then refine the declaration that already exists in packet_filter_api.h:

bool ParseFilterConfig(std::string s, SrtFilterConfig& w_config);

@jlsantiago0
Copy link
Contributor Author

@maxsharabayko Well the current one line change fixes the build. That should be merged and then the cleanup/refactor would be a different issue to be addressed later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants