Skip to content

Add cxx_virtual_method instruction.#26658

Closed
pschuh wants to merge 1 commit intoswiftlang:masterfrom
pschuh:cpp-9
Closed

Add cxx_virtual_method instruction.#26658
pschuh wants to merge 1 commit intoswiftlang:masterfrom
pschuh:cpp-9

Conversation

@pschuh
Copy link
Copy Markdown
Contributor

@pschuh pschuh commented Aug 14, 2019

This also contains the code to trigger forming cxx_virtual_method instructions but the this pointer adjustment is not hooked up yet.

Companion pr: apple/swift-clang#349

@jrose-apple
Copy link
Copy Markdown
Contributor

Hmm. I'm sad that we need cxx_virtual_method at all because it does not bode well for adding other importers in the future. I'll take a closer look soon.

@jrose-apple jrose-apple requested a review from jckarter August 15, 2019 01:03
@pschuh
Copy link
Copy Markdown
Contributor Author

pschuh commented Aug 15, 2019

Hi @jrose-apple, the context for needing cxx_virtual_method is here: https://forums.swift.org/t/c-interop-virtual-function-calls/27567/3 in case you missed it.

@jrose-apple
Copy link
Copy Markdown
Contributor

I guess if we have a C++ abstraction pattern we can have a cxx_virtual_method instruction.

@Azoy
Copy link
Copy Markdown
Contributor

Azoy commented Aug 15, 2019

I'm just a simple bystander, but could you add some SILGen tests as well?

@pschuh pschuh force-pushed the cpp-9 branch 2 times, most recently from 7b846ff to c468172 Compare August 15, 2019 21:16
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a better way to have exactly two results than this. My SIL isn't strong enough. @gottesmm?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rjmccall or @jckarter should probably verify this rather than me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems important…

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When you do get to it, we'll need some way to indicate that the adjusted "this" pointer depends on the lifetime of the original object. I'm not sure if there's a way to do that other than the mark_dependence instruction, but there ought to be.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My suggestion (going back to what I was saying above) is that the method should take self either forwarding or guaranteed.

Copy link
Copy Markdown
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

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

Before you land this, I want to take another look through. But some initial comments.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the wrong semantics. The right semantics IMO are that this should be either forwarding or guaranteed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My suggestion (going back to what I was saying above) is that the method should take self either forwarding or guaranteed.

@shahmishal
Copy link
Copy Markdown
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@shahmishal shahmishal closed this Oct 5, 2020
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