Skip to content

Fix placement of comments on constants#1383

Merged
gpetiot merged 4 commits intoocaml-ppx:masterfrom
gpetiot:cmt-on-constant
Jun 12, 2020
Merged

Fix placement of comments on constants#1383
gpetiot merged 4 commits intoocaml-ppx:masterfrom
gpetiot:cmt-on-constant

Conversation

@gpetiot
Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot commented May 28, 2020

Related to #1323 but insufficient to fix it somehow. No diff with test_branch.

Copy link
Copy Markdown
Collaborator

@jberdine jberdine left a comment

Choose a reason for hiding this comment

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

Looks good!

@gpetiot
Copy link
Copy Markdown
Collaborator Author

gpetiot commented Jun 10, 2020

I'm waiting a bit before merging it, as #1387 can make it obsolete (having the loc in the ast instead of inferring them)

@hhugo
Copy link
Copy Markdown
Collaborator

hhugo commented Jun 10, 2020

Note that the loc in the ast will only be valid with ocaml >= 4.10.

@hhugo
Copy link
Copy Markdown
Collaborator

hhugo commented Jun 10, 2020

I wonder if we should lex the full source once when calling Source.create instead of lexing bits and pieces multiple times..

@gpetiot
Copy link
Copy Markdown
Collaborator Author

gpetiot commented Jun 10, 2020

Note that the loc in the ast will only be valid with ocaml >= 4.10.

So should we merge this PR and ignore the constant locations in #1387?

@jberdine
Copy link
Copy Markdown
Collaborator

I would not object to requiring ocaml >= 4.10. Or to state that formatting is suboptimal in such cases when ocaml < 4.10 is used.

@jberdine
Copy link
Copy Markdown
Collaborator

I wonder if we should lex the full source once when calling Source.create instead of lexing bits and pieces multiple times..

I'd guess that would be faster. I have no idea if this is currently significant to performance though.

@gpetiot gpetiot merged commit 1c4923d into ocaml-ppx:master Jun 12, 2020
@gpetiot gpetiot deleted the cmt-on-constant branch June 12, 2020 09:05
@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Jun 29, 2020

FYI, in the read/format/output part, the profile is about:

  • parsing: 13%
  • comment handling (Cmts.init): 33%
  • formatting + box layout: 46%

And calls to Source functions that reparse the file are barely visible.

@jberdine
Copy link
Copy Markdown
Collaborator

Interesting, thanks @emillon! The interval tree data structure was a quick and slow implementation, and it shows. Regarding the effect that queries to Source have though, I think that the story is more involved. In particular, since we have to keep reformatting in a loop until a fixed point is reached, the cost of a use of Source is not immediately clear. In particular, if it can lead to another entire iteration (or more) of the format-parse loop, then the cost is much higher than what raw perf data would indicate. It is this sort of cost that I am more worried about, rather than the efficiency of the token parsing itself, which is not free but also not bad. Basically, the uses of Source are the point where we introduce backtracking that Format does not natively support, which is what takes the algorithm from polynomial time (Format) to exponential time (like pretty and friends).

emillon added a commit to emillon/opam-repository that referenced this pull request Aug 6, 2020
CHANGES:

#### Changes

  + Do not break inline elements such as `{i blah}` in docstrings (ocaml-ppx/ocamlformat#1346, @jberdine)

  + Distinguish hash-getter from hash-comparison infix operators. Operators of the form `#**#` or `#**.` where `**` can be 0 or more operator chars are considered getter operators and are not surrounded by spaces, as opposed to regular infix operators (ocaml-ppx/ocamlformat#1376, @gpetiot)

  + Type constraint on return type of functions is now always printed before the function body (ocaml-ppx/ocamlformat#1381, ocaml-ppx/ocamlformat#1397, @gpetiot)

#### Bug fixes

  + Restore previous functionality for pre-post extension points (ocaml-ppx/ocamlformat#1342, @jberdine)

  + Fix extra break before `function` body of a `fun` (ocaml-ppx/ocamlformat#1343, @jberdine)
    Indent further args of anonymous functions (ocaml-ppx/ocamlformat#1440, @gpetiot)

  + Do not clear the emacs `*compilation*` buffer on successful reformat (ocaml-ppx/ocamlformat#1350, @jberdine)

  + Fix disabling with attributes on OCaml < 4.08 (ocaml-ppx/ocamlformat#1322, @emillon)

  + Preserve unwrapped comments by not adding artificial breaks when `wrap-comments=false` and `ocp-indent-compat=true` are set to avoid interfering with ocp-indent indentation. (ocaml-ppx/ocamlformat#1352, @gpetiot)

  + Break long literal strings at the margin (ocaml-ppx/ocamlformat#1367, @gpetiot)

  + Break after a multiline argument in an argument list (ocaml-ppx/ocamlformat#1360, @gpetiot)

  + Remove unnecessary parens around object (ocaml-ppx/ocamlformat#1379, @gpetiot)

  + Fix placement of comments on constants (ocaml-ppx/ocamlformat#1383, @gpetiot)

  + Do not escape arguments of some Odoc tags (ocaml-ppx/ocamlformat#1391, 1408, @gpetiot, @Julow)
    The characters `[]{}` must not be escaped in the arguments of `@raise`, `@author`, `@version` and others.

  + Fix missing open line between multi-line let-binding with poly-typexpr (ocaml-ppx/ocamlformat#1372, @jberdine)

  + Remove trailing space after expression when followed by an attribute and break before attributes attached to multi-line phrases (ocaml-ppx/ocamlformat#1382, @gpetiot)

  + Do not add a space to minimal comments `(* *)`, `(** *)` and `(*$ *)` (ocaml-ppx/ocamlformat#1407, @gpetiot)

  + Fix attributes position in labelled arguments type (ocaml-ppx/ocamlformat#1434, @gpetiot)

  + Add missing parens around type annotation in anonymous function (ocaml-ppx/ocamlformat#1433, @gpetiot)

  + Fix alignment of 'then' keyword in parenthesised expression (ocaml-ppx/ocamlformat#1421, @gpetiot)

#### New features

  + Support quoted extensions (added in ocaml 4.11) (ocaml-ppx/ocamlformat#1405, @gpetiot)

  + Recognise eliom file extensions (ocaml-ppx/ocamlformat#1430, @jrochel)
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