Skip to content

Tweaks to Trivia::findNodeWhereRangeFitsIn #3169

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 27, 2025
Merged

Tweaks to Trivia::findNodeWhereRangeFitsIn #3169

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/Fantomas.Core/Trivia.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,9 @@ let rec findNodeWhereRangeFitsIn (root: Node) (range: range) : Node option =
// The more specific the node fits the selection, the better
let betterChildNode =
root.Children
|> Array.choose (fun childNode -> findNodeWhereRangeFitsIn childNode range)
|> Array.tryHead
|> Array.tryPick (fun childNode -> findNodeWhereRangeFitsIn childNode range)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the real improvement, is it not?


match betterChildNode with
| Some betterChild -> Some betterChild
| None -> Some root
betterChildNode |> Option.orElseWith (fun () -> Some root)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think that would perform better? Or this is more of a "I would have written it that way" thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Some betterChild -> Some betterChild appears to be allocatiing lots of extra instances of Option, which orElseWith avoids.
Maybe just doing Some _ -> betterChildNode would work as well

Copy link
Contributor

Choose a reason for hiding this comment

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

appears to be allocatiing

Why are you getting that from? The before and after seem rather close?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Visual Studio allocation profiler shows this with the match:
image
and this with orElseWith
image
And BDN itself say it reduces allocations from 155.24MB to 155.17MB.
Admittedly this is only a small change in memory given the number of allocations

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, thanks for sharing!


let triviaBeforeOrAfterEntireTree (rootNode: Node) (trivia: TriviaNode) : unit =
let isBefore = trivia.Range.EndLine < rootNode.Range.StartLine
Expand Down
Loading