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

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

Merged
merged 1 commit into from
Aug 8, 2016

Conversation

KiChjang
Copy link
Member

@KiChjang KiChjang commented Aug 8, 2016

Part of #35233.
Extension of #35191.

r? @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Collaborator

bors commented Aug 8, 2016

📌 Commit 06133c5 has been approved by GuillaumeGomez

@bors
Copy link
Collaborator

bors commented Aug 8, 2016

⌛ 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
Copy link
Member

nrc commented Aug 8, 2016

@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
Copy link
Contributor

sophiajt commented Aug 8, 2016

@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