-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Fix all warnings. #162
Fix all warnings. #162
Conversation
There are some left-over warnings:
These can go only if we drop some code for OCaml <= 3.11.. I'm in favor of doing it.. |
@@ -67,7 +67,7 @@ let file_deco filename = | |||
let fill frame = | |||
let pos = Frame.position frame in | |||
let duration = Frame.seconds_of_master (size-pos) in | |||
let rec aux t' = | |||
let rec aux () = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you need to keep this warning (because it says something useful: this piece of code is unfinished) or do some more modification. It may be okay to get rid of t' because the reference to t is used to keep track of time. I don't know why there is a reference, maybe to use it at the end after aux has returned.. but right now, the last update is useless because the ref is forgotten right after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what last update you have in mind but after re-reading this code, it looks fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the code again. It didn't make sense at first, but now it does, and I believe that my previous comment was misleaded. The reference t keeps track of the time in the metadata stream, and lives through all calls of fill. I don't see any problem with this code. Thanks!
let skip_header f c = read_header f c | ||
|
||
let sample w buf pos len= | ||
let sample w buf len= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the specification, it's a bug if pos is not used, it should...
@toots would it be difficult to separate trivial stuff from all those changes that we commented on? It'd be nice to get the obvious changes out of the way to discuss the functions that need real fixing. Also, I'll try to compile the branch on a relatively old OCaml. I believe I have 3.12 on my laptop, which doesn't have a Debian yet. It does not issue as many warnings as 4.0, but we should make sure there's no new syntax in the diff or anything like that. |
@dbaelde yeah, I'll do that later today. I'm all for |
I'd be fine with |
I'm okay to support only OCaml >=3.12. However, I'd like it if this branch didn't accumulate more changes. Perhaps drop the compatibility code in master and merge back? By the way I just tested a 3.12 build here and it worked fine. |
Conflicts: src/tools/harbor.ml src/tools/wav_aiff.ml
Conflicts: src/tools/harbor.ml
git version, next: make init; make update File "tools/utils.ml", line 109, characters 18-19: |
Thanks for the review! Indeed, some optional parts were not enabled locally here. I still haven't tested alsa and pulseaudio... I've fixed the above warnings, except those that need further discussion/coding.. |
Okay, ready for the final merge! |
Thanks for the work toots, it all looks fine to me! |
No description provided.