-
Notifications
You must be signed in to change notification settings - Fork 137
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
Allow non-static sources #236
Conversation
Thank you, this looks useful! I've tagged this for the next sem-ver breaking release. |
40ea366
to
d2ca4f6
Compare
Rebased onto |
a264943
to
0209f27
Compare
Hi @pdeljanov ! If you eventually want to merge this, could you do it quickly please ? This PR touches a lot of parts of the codebase, so changes on The clippy failure is unrelated to the PR, it fails on the previous commit too. |
Hi @a1phyr, Thanks for diligently updating your PR! I was pushing out a large backlog of changes I had for 0.6, but I'll pause that until we finish with your PR. It looks like a new clippy warning has appeared after the latest Rust release, but I'll just waive that since it's unrelated to your changes. I'll try to review this PR sometime in the next few days, but from my initial look-through it seems reasonable enough. |
Now that I had a chance to sit down with this, I think for the original intent, this is good as-is. However, I had a thought. Would it be better instead to make the readers generic across I understand this is also probably outside the scope you had intended for this PR. I would be happy to merge your PR as-is and I can follow up with another commit, but if you'd like to try and make the change, just let me know. |
If possible, I would prefer making the readers generic across For this PR, I also made 0d177fd (but not pushed it to this PR) to avoid the generic lifetime on |
I think the main benefit is that if a user only requires one particular type of Of course, the practical overhead of this is basically 0. Likewise, assuming the user continues to use a enum SymphoniaFormatReader {
File(FormatReader<std::fs::File>),
Buffer(FormatReader<std::io::Cursor>,
} The current design basically prevents them from being able to do this. We have a number of tickets relating to
I'm not sure about this one. Is the motivation primarily for cleanliness? One issue I see is that that it's not possible to call
How do you feel about just pushing it as a second commit to this PR? If it doesn't work out we can just drop the commit. Usually I just squash all PRs down at merge time so it'll keep the history clean. |
It is also to avoid having a useless lifetime parameter to
I found a way to keep
I tried doing that but it requires far more work as it needs to touch most functions of the codebase, and would also requires some redesign (many functions actually depend on |
Hi @pdeljanov ! Does the current state satisfy you ? |
Hi @a1phyr, I haven't forgotten about this! Due to real-world reasons, I haven't been able to dedicate any meaningful time to Symphonia. I still would like to prototype and explore being generic over a I'm still open to merging the PR as it was prior to the |
Hi @a1phyr, After experimenting with making I will look into reworking |
This allows reading data from borrowed buffers and such.
This is a breaking change, because it adds many lifetimes in codebase.
Closes #117