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

Eliminated ref_t. Some more convention fixes #1045

Merged
merged 5 commits into from Jan 20, 2020

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Jan 3, 2020

Fixes #943, #945

The new convention for passing parameters by writable reference applied. The use of ref_t and Ref has been eliminated, as well as the definitions thereof.

The new convention is:

  1. Reference parameters must have w_ prefix and pointer parameters - pw_ prefix.
  2. Passing any expression that resolves to a reference to a writable object requires (extra_parentheses) around the whole expression. This applies to:
    • passing single variable to a function
    • passing a field of a structure
    • passing a pointer to a writable object, both as pointer or as reference
    • assigning a pointer to a pointer-type field of another structure to be later passed to a function for writing
    • regardless if this reference variable is a local variable, or a parameter pass-through

The above rules don't embrace passing a non-writable object, usually preferred as const, but also when it's required to be passed by writable pointer or reference, but the function doesn't modify it.

Also the order of parameters is changed to the convention - writable reference parameters should be placed after value parameters.

@ethouris ethouris added [core] Area: Changes in SRT library core Priority: High Status: Review Needed Type: Maintenance Work required to maintain or clean up the code labels Jan 3, 2020
@maxsharabayko
Copy link
Collaborator

Looks like it also depends on PR #1028. At least I see some function from that not yet merged PR in this one.

@ethouris
Copy link
Collaborator Author

Purged of any premature merges. Now the PR is completely independent.

@mbakholdina mbakholdina added this to Backlog in Development via automation Jan 17, 2020
@mbakholdina mbakholdina added this to the v1.5.0 milestone Jan 17, 2020
@mbakholdina mbakholdina moved this from Backlog to Needs Review in Development Jan 17, 2020
Comment on lines 226 to 227
tuple<string, string> ExtractPath(string path)
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like something from another PR.

Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Ideally, this is better to be divided into two commits: one for changes to core, another for changes to the apps.
This can be easily done with two git commands:

git reset @~3
git add ./srtcore/*
git commit -m "[core] Eliminated ref_t from the core"
git add ./apps/*
git add ./testing/*
git commit -m "[apps] Eliminated ref_t from apps"
git push --force

apps/srt-file-transmit.cpp Outdated Show resolved Hide resolved
apps/srt-file-transmit.cpp Outdated Show resolved Hide resolved
srtcore/core.cpp Show resolved Hide resolved
srtcore/core.cpp Outdated Show resolved Hide resolved
srtcore/queue.cpp Outdated Show resolved Hide resolved
@ethouris
Copy link
Collaborator Author

I understand, but the trouble is that splitting makes sense if they can be independent - whereas as this involves deletion of ref_t and Ref, this change applies to both, or at least one would be dependent on the other and require a blocking merge cycle.

@maxsharabayko
Copy link
Collaborator

Well, it is simple to solve. The first commit should remove ref_t from apps, the second - from the core.

@maxsharabayko
Copy link
Collaborator

So like this then

git reset @~3
git add ./apps/*
git add ./testing/*
git commit -m "[apps] Eliminated ref_t from apps"
git add ./srtcore/*
git commit -m "[core] Eliminated ref_t from the core"
git push --force

Copy link
Collaborator

@rndi rndi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must say these new prefixes do look silly. Also, experience tells me they are going to be hard to enforce or maintain. But no (serious) harm done and this is definitely better than 'Ref()' usage.

Other than that, please resolve the merge conflict with the master branch. Thanks!

@rndi rndi merged commit c6c29f8 into Haivision:master Jan 20, 2020
Development automation moved this from Needs Review to Done Jan 20, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0, v1.4.2 Oct 14, 2020
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 Priority: High Type: Maintenance Work required to maintain or clean up the code
Projects
No open projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

Replace the use of Ref with dedicated variables with prefix
4 participants