Skip to content

Lengthen the span label to include match and expr for E0004#35485

Merged
bors merged 1 commit into
rust-lang:masterfrom
KiChjang:e0004-bonus
Aug 8, 2016
Merged

Lengthen the span label to include match and expr for E0004#35485
bors merged 1 commit into
rust-lang:masterfrom
KiChjang:e0004-bonus

Conversation

@KiChjang

@KiChjang KiChjang commented Aug 8, 2016

Copy link
Copy Markdown
Contributor

Part of #35233.
Extension of #35191.

r? @GuillaumeGomez

@GuillaumeGomez

Copy link
Copy Markdown
Member

Thanks!

@bors: r+ rollup

@bors

bors commented Aug 8, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 06133c5 has been approved by GuillaumeGomez

@bors

bors commented Aug 8, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 06133c5 with merge 8a4641b...

bors added a commit that referenced this pull request Aug 8, 2016
Lengthen the span label to include match and expr for E0004

Part of #35233.
Extension of #35191.

r? @GuillaumeGomez
@bors bors merged commit 06133c5 into rust-lang:master Aug 8, 2016
@KiChjang KiChjang deleted the e0004-bonus branch August 8, 2016 15:36
@nrc

nrc commented Aug 8, 2016

Copy link
Copy Markdown
Member

@GuillaumeGomez, @KiChjang Manually altering spans like this is not usually a good idea - you have no guarantee that the new lo point will be before the new hi point in the source text due to macro expansion. In general, I would say just never do this - you have to put up with the spans you have. in the future, we should have more spans plus we'll provide infrastructure to concat them. In the meantime, if you must do this then you need to check that the span you create is valid.

I'm also not sure about re-using the expansion id, given that the rest of the expression might have a different expansion history, it doesn't make a lot of sense. Seeing as this span should just be used for error messages, you can probably just use the 'no expansion' id.

@sophiajt

sophiajt commented Aug 8, 2016

Copy link
Copy Markdown
Contributor

@nrc - filed #35529

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.

5 participants