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

Assorted IO improvements #519

Merged
merged 4 commits into from
Jun 12, 2021
Merged

Assorted IO improvements #519

merged 4 commits into from
Jun 12, 2021

Conversation

Kha
Copy link
Member

@Kha Kha commented Jun 9, 2021

No description provided.

@@ -445,6 +445,8 @@ def run (args : SpawnArgs) : IO String := do
throw <| IO.userError <| "process '" ++ args.cmd ++ "' exited with code " ++ toString out.exitCode
pure out.stdout

@[extern "lean_io_exit"] constant exit : UInt8 → IO α
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved it into Process following Rust


def withStdin [MonadFinally m] (h : FS.Stream) (x : m α) : m α := do
def withStdin [MonadFinally IO] (h : FS.Stream) (x : IO α) : IO α := do
Copy link
Member

Choose a reason for hiding this comment

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

The [MonadFinally IO] argument seems unnecessary now. withStdin also seems more useful in the m α → m α version.

let mode :=
match m with
match IO with
Copy link
Member

Choose a reason for hiding this comment

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

Overzealous search&replace? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes :)

Kha and others added 4 commits June 9, 2021 12:24
…into `IO.FS`

Automatic lifting takes care of this, and it wasn't consistently applied anyway
The names were taken from `llvm-config`
@tydeu
Copy link
Member

tydeu commented Jun 9, 2021

Since you added --print-prefix and --print-libdir (in 1dbf898), maybe add --print-bindir as well for parity's sake?

@tydeu
Copy link
Member

tydeu commented Jun 9, 2021

Also, would this be a good time to make def IO.RealWorld : Type := Unit, constant IO.RealWorld : Type := Unit?

@Kha
Copy link
Member Author

Kha commented Jun 9, 2021

Also, would this be a good time to make def IO.RealWorld : Type := Unit, constant IO.RealWorld : Type := Unit?

I believe that could negatively influence the generated code.

edit: rather, see most recent change of that line

@tydeu
Copy link
Member

tydeu commented Jun 10, 2021

@Kha

Ah, thanks! That explains it. I am surprised, though, that unsafe code can't see through a defined constant. Also, any feedback about the proposed --print-bindir addition?

@leodemoura leodemoura merged commit 2091a09 into leanprover:master Jun 12, 2021

@[inline]
def withFile (fn : FilePath) (mode : Mode) (f : Handle → m α) : m α :=
def withFile (fn : FilePath) (mode : Mode) (f : Handle → IO α) : IO α :=
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reasoning for applying this change to functions that take monadic callbacks? I often used withFile with an f that does not return IO. What is the recommended workaround?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I had reverted all higher-order uses of m, but apparently not. Will revert as soon as the tests pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this function is a bit weird in that I don't see any real advantage of using it in favor of using Handle.mk directly. We don't give any formal guarantees but it should be reasonable to assume that non-borrowed variables are freed (decremented) as soon as possible. And withFile doesn't guarantee that the file is closed after executing f either since the handle could be passed back in the return type. But perhaps it still gives a kind of peace of mind? @leodemoura Am I missing something?

ChrisHughes24 pushed a commit to ChrisHughes24/lean4 that referenced this pull request Dec 2, 2022
Another complete rewrite. This implements one `ring` to rule them all: it incorporates both `ring_exp` and `ring` behaviors, now under the name `ring`. (`ring_exp` had some small (1.4x) performance issues that prevented it from being used by default; I'm hoping that those issues are fixed now, and we can revisit later if it becomes an issue.)

Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
ChrisHughes24 pushed a commit to ChrisHughes24/lean4 that referenced this pull request Dec 2, 2022
TODO:
* Before merge
- [x] fix a bug in linarith in mathlib3 I just found ...
- [x] depends on: leanprover#519 
- [x] style lint
- [x] docs
- [X] move theory stubs to a separate PR, for easier tracking: leanprover#733 
- [x] failing to parse the `LinarithConfig` option

* Before or after merge?
- [ ] Implement the `removeNe` preprocessor.
- [ ] Add support for restricting to a single type. How to store a `Type` in `LinarithConfig`?

* After merge
- [ ] Teach `norm_num` to solve `example [LinearOrderedRing α] : (0 : α) < 37 := by norm_num`.
- [ ] Port `zify_proof` (plumbing for `zify`), and add the `natToInt` preprocessor.
  Mostly done now, but see leanprover#741 before all the tests will work.
- [ ] Port `cancel_denoms` tactic, and add the `cancelDenoms` preprocessor.
- [ ] Add the `nlinarith` preprocessor and front-end syntax.


Co-authored-by: Mario Carneiro <di.gama@gmail.com>
Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
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

5 participants