Skip to content

Remove struct_span_err function from ExtCtxt #76518

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

Conversation

GuillaumeGomez
Copy link
Member

@rust-highfive
Copy link
Contributor

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2020
@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 9, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 9, 2020

FYI this wasn't what I was referring to in #76406 - Session::struct_span_err, confusingly, is different from struct_span_err! - the second takes an error code and the first doesn't.

No opinion on this PR, though.

@GuillaumeGomez
Copy link
Member Author

Yep, the naming is funny. I don't want to update the macro though (because I'd prefer that we used error codes everywhere \o/).

@pickfire
Copy link
Contributor

pickfire commented Sep 9, 2020

Weird, I thought the struct_span_error! macro can take in an optional error code so we can use that here but it doesn't seemed so. Oh, or maybe we should have struct_span_error renamed as struct_span_error_without_code, hehe more troublesome means more likely people will write an error code.

@pickfire
Copy link
Contributor

pickfire commented Sep 9, 2020

Yep, the naming is funny. I don't want to update the macro though (because I'd prefer that we used error codes everywhere \o/).

Just wondering, if we write error codes everywhere doesn't mean later we will have a high chance to duplicate one of the error code?

@GuillaumeGomez
Copy link
Member Author

There is a check for that. You can't use the same error code at multiple places.

@pickfire
Copy link
Contributor

pickfire commented Sep 9, 2020

There is a check for that. You can't use the same error code at multiple places.

Not same error code. Different error code but a same or a very similar message.

@estebank
Copy link
Contributor

There is a check for that. You can't use the same error code at multiple places.

We no longer to 👀

Not same error code. Different error code but a same or a very similar message.

This already happens from time to time. Sometimes we leave it like that, sometimes we merge them.

@estebank
Copy link
Contributor

I don't see the benefit this PR brings, but I don't see its harm either. What's the rationale? If I understand correctly it was a comment about using the struct_span_error! macro, not about removing ExtCtxt::struct_span_err, right?

@GuillaumeGomez
Copy link
Member Author

Yes, not really useful... Closing it then.

@GuillaumeGomez GuillaumeGomez deleted the remove-unneeded-extctxt-fn branch September 10, 2020 17:51
@pickfire
Copy link
Contributor

@estebank Since we are not using struct_span_error_with_code and we used struct_span_error! instead, but we still have struct_span_error delegating to the original struct_span_error, I thought it would be better to remove struct_span_error so it does not cause confusion as there is no struct_span_error_with_code.

@estebank
Copy link
Contributor

@pickfire I would like to do that sort of clean up in a more mechanical/principled way, checking what patterns we currently use and optimize for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up rustc_expand: remove struct_span_err method and use the macro instead
5 participants