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

Make it possible to track the original text range #18387

Closed
wants to merge 3 commits into from

Conversation

ijklam
Copy link
Contributor

@ijklam ijklam commented Mar 15, 2025

Description

Make it possible to track the original text range after #line directive supplied.

Used to support #17519

Checklist

  • Test cases added
  • Performance benchmarks added in case of performance changes
  • Release notes entry updated:

@ijklam ijklam requested a review from a team as a code owner March 15, 2025 14:38
Copy link
Contributor

github-actions bot commented Mar 15, 2025

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.300.md

@Martin521
Copy link
Contributor

Martin521 commented Mar 15, 2025

I had considered exactly this for #18049 - it would simplify that work considerably. And the current Range design (which just throws away the real (original) range) is really bad.
But I considered it too impactful a change in this basic type, where a lot of compute effort is spent to keep it to 64 bit, and now we would more than double the size.
I am curious about the reaction of the team.

Comment on lines +265 to +266
[<System.Diagnostics.DebuggerDisplay("{OriginalRange} -> ({StartLine},{StartColumn}-{EndLine},{EndColumn}) {ShortFileName} -> {DebugCode}")>]
type Range private (code1: int64, code2: int64, originalRange: struct (int64 * int64) voption) =
Copy link
Member

Choose a reason for hiding this comment

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

This is more than doubling the size of a very low-level structure used throughout most representations used in the compiler.

Considering #line a rather edge feature targeting code generators, I don't see this increase well justified.

Given the #line feature usage, I would rather sacrifice UX when #line is encountered at the referenced feature.

@T-Gro
Copy link
Member

T-Gro commented Mar 17, 2025

I had considered exactly this for #18049 - it would simplify that work considerably. And the current Range design (which just throws away the real (original) range) is really bad. But I considered it too impactful a change in this basic type, where a lot of compute effort is spent to keep it to 64 bit, and now we would more than double the size. I am curious about the reaction of the team.

I think the approached you ended up at #18049 makes the right compromise - the only size/perf hit is when #line is actually used, and regular F# code is not paying for it. The LineMaps data structure could fit here well IMO.

@ijklam
Copy link
Contributor Author

ijklam commented Mar 17, 2025

I had considered exactly this for #18049 - it would simplify that work considerably. And the current Range design (which just throws away the real (original) range) is really bad. But I considered it too impactful a change in this basic type, where a lot of compute effort is spent to keep it to 64 bit, and now we would more than double the size. I am curious about the reaction of the team.

I think the approached you ended up at #18049 makes the right compromise - the only size/perf hit is when #line is actually used, and regular F# code is not paying for it. The LineMaps data structure could fit here well IMO.

Ok, I see it. I will close this and revert it from #17519.

@ijklam ijklam closed this Mar 17, 2025
@ijklam ijklam deleted the track-original-range branch March 17, 2025 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants