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

don't suppress deprecation warnings (3) #387

Closed
wants to merge 1 commit into from

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Jan 5, 2019

as noticed in mirage/mirage-net-xen#79 (comment) - I'm actually in favour of not suppressing warnings to remind ourselves that we need to work on this... Lwt_sequence getting deprecated as tracked in mirage/mirage#889

@avsm
Copy link
Member

avsm commented Jan 6, 2019

The issue is that this breaks the default warnings-as-errors mode of dune (which is otherwise reasonable during development). The issue with the Lwt_sequence deprecation is that there's no clear solution to it yet, so the function is marked as deprecated but can't actually be immediately fixed (without pulling out Lwt_sequence into a separate package). So I'd prefer to keep track of these sorts of warnings in issue (as done in mirage/mirage#889) and suppress the deprecation warning until we're ready to fix it.

@emillon
Copy link

emillon commented Jan 7, 2019

An in-between solution is to explicitly mark the problematic modules with:

[@@@ocaml.warning "-3"]
module Lwt_sequence = Lwt_sequence (* see issue #xxx *)
[@@@ocaml.warning "+3"]

BTW do you know if there's a plan to extract the existing Lwt_sequence to opam? A verified version would be nice but it does not seem necessary to wait for it.

@avsm
Copy link
Member

avsm commented Jan 7, 2019

BTW do you know if there's a plan to extract the existing Lwt_sequence to opam? A verified version would be nice but it does not seem necessary to wait for it.

I did a quick extraction to https://github.com/mirage/lwt-dllist -- as soon as there is agreement on #349 i can cut a release and we can use that instead of the builtin version.

@avsm
Copy link
Member

avsm commented Jan 14, 2019

fixed by #388 instead

@avsm avsm closed this Jan 14, 2019
@hannesm hannesm deleted the no-suppression-of-3 branch January 14, 2019 19:09
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.

3 participants