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

save_analysis: various fixes #73071

Closed

Conversation

marmeladema
Copy link
Contributor

Fixes #73020
Fixes #73022
Fixes #73041

r? @Xanewok

Once approved, could it be never rollup'ed? Because there was some strange issues happening on windows during CI.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2020
@jonas-schievink
Copy link
Contributor

@bors rollup=never

@RalfJung
Copy link
Member

RalfJung commented Jun 6, 2020

This overlaps with #73046. I am at a loss as to why @marmeladema opened a second PR instead of continuing work on the first.

But also, the strange issues are likely actually caused by some other PR, as a rollup without #73046 showed the same problem -- I guessed wrong when trying to figure out why the rollup broke. So the two extra commits here might or might not be needed.

value: make_signature(decl, generics),
value: fn_to_string(
decl,
hir::FnHeader {
Copy link
Member

Choose a reason for hiding this comment

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

That's unfortunate, together with importing rustc_target as a dependency...

I think this would be alleviated if we stored FnSig that has both FnHeader and FnDecl in ForeignItemKind::Fn and accessed that directly rather than just synthesizing the header (we lose the extern ABI information, in particular). The header is small and Copy so hopefully this doesn't impact things much...

Would such a change to HIR make sense? @RalfJung since you've already invested a bit of time in this PR series 😅

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I know nothing about HIR.^^

Copy link
Member

Choose a reason for hiding this comment

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

Let's try then... @petrochenkov

@marmeladema
Copy link
Contributor Author

marmeladema commented Jun 6, 2020

I opened a second PR because you commented on the first one saying it was responsible for the build failures and I hoped the additional commits might fix it. Sorry for the confusion, I am just trying to be helpful.

Let's close this one for the time being. I'll rebase and re-open once #73046 is actually merged.

@marmeladema marmeladema closed this Jun 6, 2020
@RalfJung
Copy link
Member

RalfJung commented Jun 6, 2020

I opened a second PR because you commented on the first one saying it was responsible for the build failures and I hoped the additional commits might fix it. Sorry for the confusion, I am just trying to be helpful.

It's okay, misunderstandings happen. :)
It's totally fine to add additional commits like that to an existing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
5 participants