Skip to content

Move file_id into Span#363

Open
LesterEvSe wants to merge 1 commit into
BlockstreamResearch:masterfrom
LesterEvSe:feature/span-with-file-id
Open

Move file_id into Span#363
LesterEvSe wants to merge 1 commit into
BlockstreamResearch:masterfrom
LesterEvSe:feature/span-with-file-id

Conversation

@LesterEvSe

@LesterEvSe LesterEvSe commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR moves file_id into Span as a structural component of the source location. It also removes the unnecessary file_id and set_file_id methods from the parse.rs file.
Refactored the driver code a bit

@LesterEvSe LesterEvSe requested a review from KyrylR June 25, 2026 10:19
@LesterEvSe LesterEvSe self-assigned this Jun 25, 2026
@LesterEvSe LesterEvSe added the enhancement New feature or request label Jun 25, 2026
@LesterEvSe LesterEvSe requested a review from delta1 as a code owner June 25, 2026 10:19
@LesterEvSe LesterEvSe marked this pull request as draft June 25, 2026 10:19
@LesterEvSe LesterEvSe force-pushed the feature/span-with-file-id branch from 539748d to 37ab542 Compare June 25, 2026 10:23
@LesterEvSe LesterEvSe linked an issue Jun 25, 2026 that may be closed by this pull request
Refactor(ErrorHandler): Move file_id into Span and compute it during parsing #347
Open
@LesterEvSe LesterEvSe marked this pull request as ready for review June 25, 2026 10:26
KyrylR
KyrylR reviewed Jun 25, 2026
View reviewed changes
Comment thread src/driver/mod.rs
Comment thread src/driver/mod.rs

/// Since `ids` and `paths` are equal, we can return len one of them
fn len(&self) -> usize {
self.paths.len()

@KyrylR KyrylR Jun 25, 2026

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.

Maybe we should have a guard here to make sure that ids and paths are equal?

@LesterEvSe LesterEvSe Jun 25, 2026

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.

Add debug_assert_eq! inside the insert function to verify this

Comment thread src/driver/mod.rs
Comment thread src/driver/mod.rs
Comment thread src/driver/mod.rs Outdated
@apoelstra

apoelstra commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

37ab542 needs rebase

@LesterEvSe LesterEvSe force-pushed the feature/span-with-file-id branch from 37ab542 to ebf0d21 Compare June 25, 2026 14:41
@apoelstra

apoelstra commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

ebf0d21 needs rebase

@LesterEvSe LesterEvSe force-pushed the feature/span-with-file-id branch from ebf0d21 to c81d8d0 Compare June 26, 2026 14:24
@apoelstra

apoelstra commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

In c81d8d0:

Can wait for another PR, but we should also change Span::new to take a usize for the file_id and a std::ops::Range rather than two usizes for the span. This should go a long way toward preventing mistakes when calling the constructor. It would also emphasize that the span's range is half-open.

Sadly we can't store a range inside Span because range isn't Copy, for dumb historic reasons. But we can at least take one in the constructor.

@LesterEvSe

LesterEvSe commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Hm, interesting; imo it would be better to make Iterator Copy, but it is what it is. I will change the constructor for this

@LesterEvSe LesterEvSe force-pushed the feature/span-with-file-id branch from c81d8d0 to 7b6db6c Compare June 29, 2026 09:35
apoelstra
apoelstra approved these changes Jun 29, 2026
View reviewed changes

@apoelstra apoelstra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK 7b6db6c; successfully ran local tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KyrylR KyrylR KyrylR left review comments
@apoelstra apoelstra apoelstra approved these changes
@delta1 delta1 Awaiting requested review from delta1 delta1 is a code owner

Assignees

@LesterEvSe LesterEvSe

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor(ErrorHandler): Move file_id into Span and compute it during parsing

3 participants

@LesterEvSe @apoelstra @KyrylR