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

Avoid keeping the SourceText instance around in SyntaxTree #45

Closed
alexrp opened this issue Mar 1, 2023 · 1 comment
Closed

Avoid keeping the SourceText instance around in SyntaxTree #45

alexrp opened this issue Mar 1, 2023 · 1 comment
Labels
area: analysis Issues related to language analyses. area: build Issues related to the build system. area: driver Issues related to the command line driver. type: feature Issues that are classified as feature requests.
Milestone

Comments

@alexrp
Copy link
Sponsor Member

alexrp commented Mar 1, 2023

public sealed class SyntaxAnalysis
{
// TODO: We should eventually get rid of this. When we need the source text, we can reconstruct it from the tree.
public SourceText Text { get; }

This is neat because, for the happy case, we don't need to access the SourceText at all. Only when there are diagnostics do we need to access line information from the SourceText, and it's reasonable to just reconstruct it for those cases.

Even then, we still have to keep the SourceText around during parsing in order to construct locations for diagnostics. To remedy that, we should probably also consider changing SourceDiagnostic to not carry a SourceLocation, but rather a SourceTextSpan and a SyntaxItem reference. A SourceDiagnostic.GetLocation() method could then be exposed which would resolve the SourceLocation.

@alexrp alexrp added state: approved Feature requests and housekeeping tasks that have been approved. type: feature Issues that are classified as feature requests. area: analysis Issues related to language analyses. labels Mar 1, 2023
@alexrp alexrp added this to the v2.0 milestone Mar 1, 2023
@alexrp alexrp self-assigned this Mar 1, 2023
alexrp added a commit that referenced this issue Mar 4, 2023
* Rename {Syntax,Semantic}Analysis -> {Syntax,Semantic}Tree.
* Rename {Syntax,Semantic}Analysis.Document -> Root.
* Rename {SyntaxItem,SemanticNode}.Analysis -> Tree.
* Rename SourceDiagnostic* types -> Diagnostic*.
* Move diagnostic types to Vezel.Celerity.Language.Diagnostics.
* Replace Diagnostic.Location property with Tree and Span properties.
* Replace DiagnosticNote.Location propert with Span property.
* Rename SourceLocation -> SourceTextLocation and make it a class.
* Remove the concept of a missing SourceTextLocation.
* {SyntaxTree,SemanticTree,LintAnalysis}.Diagnostics no longer contains
  diagnostics from earlier stages of analysis.
* Remove {Diagnostic,DiagnosticNote}.Create() methods.
* Rename LintContext.CreateDiagnostic() -> ReportDiagnostic() and take a
  SourceTextSpan instead of SourceTextLocation.
* Move DiagnosticCode.InternalError to StandardDiagnosticCodes.
* Implement ToString() on Symbol.
* Implement ToString() and ToFullString() on SemanticNode.

Part of #45.
@alexrp
Copy link
Sponsor Member Author

alexrp commented Mar 4, 2023

The diagnostic aspect has been handled, along with some related refactoring/cleanup.

At this point, I think the Text property can be turned into a GetText() method that returns the existing SourceText (if alive) or reconstructs it from the tree. It would be nice to keep an existing SourceText around as something like a SoftReference<T> so that it's not constantly recomputed, but can also still be collected if the GC is pressured.

@alexrp alexrp modified the milestones: v2.0, v1.0 Mar 4, 2023
@alexrp alexrp changed the title Avoid keeping the SourceText instance around in SyntaxAnalysis Avoid keeping the SourceText instance around in SyntaxTree Mar 4, 2023
@alexrp alexrp closed this as completed in bbc1b41 Mar 4, 2023
@alexrp alexrp added area: build Issues related to the build system. area: driver Issues related to the command line driver. and removed state: approved Feature requests and housekeeping tasks that have been approved. labels Mar 4, 2023
@alexrp alexrp removed their assignment Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: analysis Issues related to language analyses. area: build Issues related to the build system. area: driver Issues related to the command line driver. type: feature Issues that are classified as feature requests.
Development

No branches or pull requests

1 participant