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

Use actual_time to encode allow_belated argument #42

Merged
merged 2 commits into from
May 27, 2021

Conversation

mgeier
Copy link
Member

@mgeier mgeier commented May 26, 2021

This is probably a bad idea ...

But it avoids the awkwardness of the original allow_belated value potentially being invalidated.

And we don't need to define the bool type (see also #2)!

@larsoner
Copy link
Collaborator

This seems okay to me, but I'm not that familiar with the C code. Does it seem like a possibly bad idea because it's repurposing actual_time to implicitly store what was previously allow_belated?

@mgeier
Copy link
Member Author

mgeier commented May 26, 2021

Does it seem like a possibly bad idea because it's repurposing actual_time to implicitly store what was previously allow_belated?

Exactly.

Before this PR we had an explicit and appropriately named bool field, with these shortcomings:

  • it might be changed in the callback, see:

    // Due to inaccuracies in timeInfo, "diff" might have a small negative
    // value in a future block. We don't count this as "belated" though:

    The user should not look at it after the command is complete.

  • not const because of that, which would make more sense

  • a waste of space

Since actual_time is non-const and initially unused, I thought I could re-use it. This solves the mentioned problems, but it might be confusing because the name doesn't give any hint about this usage ...

I've already been using it like a bool:

if not action.actual_time:
belated += 1

... so I thought I might as well extend that usage.

@larsoner
Copy link
Collaborator

Sounds good to me, ready to merge from your end?

@mgeier
Copy link
Member Author

mgeier commented May 27, 2021

Yes!

@larsoner larsoner merged commit 6147a3a into spatialaudio:master May 27, 2021
@larsoner
Copy link
Collaborator

Thanks @mgeier

@mgeier mgeier deleted the allow-belated-hack branch May 27, 2021 17: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

2 participants