Skip to content

Fix comments' position#921

Closed
hhugo wants to merge 22 commits intomasterfrom
fix-comments
Closed

Fix comments' position#921
hhugo wants to merge 22 commits intomasterfrom
fix-comments

Conversation

@hhugo
Copy link
Copy Markdown
Collaborator

@hhugo hhugo commented Jul 9, 2019

attempt to fix #917

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Jul 12, 2019

Last regression of test_branch that I didn't manage to fix yet (with --margin=100), this doesn't stabilize after 10 iterations:

let prop_init_formals_seed fooooooooooooooooooo =
  let sigma_seed =
    create_seed_vars (* formals already there plus new ones *) (prop.Prop.sigma @ sigma_new_formals)
  in
  foooooooooooooooooooooooooo

It is margin dependent, it is well formatted for 99 and 101 but fails for 100, I think the issue is due to the fmt_args function but I didn't manage to fix it. @hhugo do you have an idea?

@gpetiot gpetiot changed the title Fix comments (WIP) Fix comments Jul 15, 2019
@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Jul 15, 2019

@hhugo I fixed the last regressions I think it's ready to be reviewed now.
@ceastlund can you test this branch on your codebase?


class a =
(* A'' *)
let open (* A' *) A (* A *) in
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is weird

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I didn't manage to fix this one, nothing seems wrong in the code.

open (* c *) struct
type t
end
(* d *) in
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

comment move inside.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is due to the new within/within heuristics, and I didn't manage to fix this without reverting to the state previous to this PR.

Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Looks like this is slowing down ocamlformat a lot !
Running dune clean; time dune runtest takes 9s on master and 1m30 with this PR.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Jul 16, 2019

I optimized a bit the changes made in Cmts.ml, but it can still be improved, any idea is welcomed.

@hhugo
Copy link
Copy Markdown
Collaborator Author

hhugo commented Jul 17, 2019

I believe I've fixed most of the performance issue.

Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

This PR is very big. Is there parts that could be moved to new PRs to make review easier ?

type usert

type usert (* a user-defined value *)
(* a user-defined value *)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These 3 comments are not placed correctly, the original code is:

module type VALUE = sig
  type value (* a Lua value *)
  type state (* the state of a Lua interpreter *)
  type usert (* a user-defined value *)
end;;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This bug is still here.

@hhugo
Copy link
Copy Markdown
Collaborator Author

hhugo commented Jul 19, 2019

This PR is very big. Is there parts that could be moved to new PRs to make review easier ?

The diff in Translation_unit can be moved in a separate PR

@gpetiot gpetiot force-pushed the fix-comments branch 2 times, most recently from 3b8e5fa to c813fd7 Compare July 26, 2019 04:49
@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Jul 30, 2019

Should revert #923 once it is merged.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Sep 2, 2019

Fixed the conflicts but it is worse than before, more work needed to the attachment algorithm.

@Julow
Copy link
Copy Markdown
Collaborator

Julow commented Feb 14, 2020

What should we do with this PR ?

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Feb 17, 2020

Hopefully #1193 will fix everything this issue tries to fix. Something weird happened to this PR I can't push commits to it anymore.

@jberdine
Copy link
Copy Markdown
Collaborator

Should this be closed or converted to a draft?

@gpetiot gpetiot marked this pull request as draft May 15, 2020 07:11
@hhugo hhugo closed this Nov 5, 2020
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.

comments moving in ways that change their meaning

5 participants