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

Rewrite has destructor analysis as a fixed-point analysis #932

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

bd339
Copy link

@bd339 bd339 commented Aug 25, 2017

Fixes #927 . Note that this creates a dependency between the "cannot derive copy" and "has destructor" analysis, i.e. the "has destructor" analysis must run before the "cannot derive copy" analysis, because "cannot derive copy" needs the results of "has destructor".

@bd339
Copy link
Author

bd339 commented Aug 25, 2017

r? @fitzgen

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @bd339 !

Just one nitpick to be addressed below and then we can merge this :)

src/ir/comp.rs Outdated
@@ -989,6 +943,11 @@ impl CompInfo {
return self.has_own_virtual_method;
}

/// Does this type have a destructor?
pub fn has_destructor(&self) -> bool {
self.has_destructor
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 rename these to either trivially_has_destructor or has_own_destructor to clarify that this isn't looking through template instantiations or base members.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Let me know when you update the PR, sometimes I don't get the new-commits-pushed emails for whatever reason. Thanks again!

Copy link
Author

Choose a reason for hiding this comment

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

Have done so, but squashed and did a force push. Maybe that's why you didn't get an email?

@fitzgen
Copy link
Member

fitzgen commented Aug 25, 2017

@bors-servo r+

@bd339 thanks a bunch! Are you interested in hacking on bindgen some more? #933 could be a good issue to pick up, if you're interested :)

@bors-servo
Copy link

📌 Commit cd41e5c has been approved by fitzgen

@bors-servo
Copy link

⌛ Testing commit cd41e5c with merge dc253cb...

bors-servo pushed a commit that referenced this pull request Aug 25, 2017
Rewrite `has destructor` analysis as a fixed-point analysis

Fixes #927 . Note that this creates a dependency between the "cannot derive copy" and "has destructor" analysis, i.e. the "has destructor" analysis must run before the "cannot derive copy" analysis, because "cannot derive copy" needs the results of "has destructor".
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing dc253cb to master...

@bors-servo bors-servo merged commit cd41e5c into rust-lang:master Aug 25, 2017
@bd339 bd339 deleted the has_destructor_fp branch August 26, 2017 05:28
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.

4 participants