这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@ccasin
Copy link

@ccasin ccasin commented Mar 5, 2023

Rebase of all Jane Street stuff (including comprehensions) onto the latest upstream ocamlformat.

@antalsz please look at two things in particular:

  1. How I merged your tests into the upstream test refactoring I previously mentioned.
  2. I moved language_extension.{ml,mli} into the ocaml-common directory rather than having copies in each parser. It looks like they were identical and didn't depend on anything that prevents this, but let me know if there was a reason for how it was.

stedolan and others added 14 commits March 3, 2023 16:56
(Note for future rebases: at this stage it is expected that `dune b
@test/passing/runtest` will give errors related to extra
`[@ocaml.curry]`s)
Note for future rebases: it is expected that `dune b
@test/passing/runtest` will fail at this stage, in my case with
output:

File "test/passing/dune.inc", line 3594, characters 0-205:
3594 | (rule
3595 |  (deps tests/.ocamlformat )
3596 |  (package ocamlformat)
3597 |  (action
3598 |   (with-stdout-to local.ml.stdout
3599 |    (with-stderr-to local.ml.stderr
3600 |      (run %{bin:ocamlformat} --margin-check %{dep:tests/local.ml})))))
Command exited with code 1.
For reference during future rebasing: `dune b @test/passing/runtest`
is not expected to succeed on this commit.  I get formatting changes
in `test/passing/tests/local.ml` where `local_` has been turned into
an attribute.
For future reference during rebasing: `dune b @test/passing/runtest`
is not expected to pass on this commit.  Some uses of `local_` are
turned into `[%extension.local]`.
For reference during future rebases: `dune b @test/passing/runtest` is
expected to pass on this commit.
For future rebases: `make test` should pass at this stage.  It may be
necessary to run `dune fmt` and `make regtests-promote` (but I checked
`dune b @test/passing/runtest` succeeded first)
Autogenerated attributes are now 'extension.foo' instead of 'ocaml.foo'
Signed-off-by: Cameron Wong <cwong@janestreet.com>
* Initial support for the include_functor extension
* Add test for include functor

Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
)

A bug in the previous patch for locals causes ocamlformat to incorrectly
format local arguments which are punned with a label and have a type signature:

    fun ~(local_ x : t) -> ...

by putting an extra invalid pair of parens around (x : t). This patch removes
the parens.

Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
)

Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
…] (oxcaml#6)

Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
@antalsz
Copy link

antalsz commented Mar 6, 2023

Re. #2: language_extensions.ml{,i} comes from ocaml-jst. I can't tell if ocaml-common/ does; if it does, then that's a fine place for language_extensions.ml{,i} too.

Copy link

@antalsz antalsz left a comment

Choose a reason for hiding this comment

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

I went through and grepped the testsuite for [: and [|, and I'm satisfied that immutable array/comprehension copies of tests occur next to all existing array tests, which is the correctness condition for adding the new tests. There's one requested change where we should use the immutable array indexing operator, but other than that the two questions for me are hereby addressed!


(... |], :].)

antalsz added 2 commits March 7, 2023 08:58
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
@ccasin ccasin force-pushed the rebase-on-upstream-95ee0a1-full branch from 2773fea to 355d694 Compare March 7, 2023 08:58
This was referenced Mar 13, 2023
@ceastlund
Copy link

Pushed a newer rebase.

@ceastlund ceastlund closed this Mar 17, 2023
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.

5 participants