Skip to content

Conversation

@pschuh
Copy link
Contributor

@pschuh pschuh commented Jul 10, 2019

Known issues so far:

  • Type-checking doesn't seem to know the c++ rules for selecting const
    vs non-const.
  • The convention for passing arguments doesn't appear correct yet.
  • Objective-c++ details are not handled yet.
  • Methods on base classes and virtual functions will not be found.
  • Const methods cannot be marked as non-mutating. This is because the
    fact that self is indirect is getting lost in the lowering process (becomes @in)

This is just some ImportDecl.cpp changes and CXXMethodConventions + the boilerplate in AbstractionPattern. There will probably need to be more to fully support methods.

@pschuh pschuh requested a review from jrose-apple July 10, 2019 00:51
Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

Just a bunch of minor comments.

@pschuh pschuh force-pushed the cpp-2 branch 2 times, most recently from 16fc18f to 026ae3c Compare July 19, 2019 21:21
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

This seems pretty straightforward, and I admit I can't comment too much on the AbstractionPattern parts. However, two things stuck out to me:

  • Where do we treat static C++ methods differently from non-static ones? Or should we treat static ones like C functions, and the AbstractionPattern for C++ methods is really "C++ instance methods"?

  • Is it possible for us to avoid adding all the new kinds of AbstractionPattern? A CXXMethodDecl is already a kind of clang::FunctionDecl, and we can already import regular functions as methods using NS_SWIFT_NAME. On the Swift side, a C++ instance method just has a different set of calling conventions but otherwise seems the same.

I admit this second part is born out of people asking about other importers, even if that's way more in the hypothetical future. I'm not going to go as far as to say everything needs to fit into the existing mold, or that you have to generalize everything so that we can implement a Fortran importer, but if we can figure out what needs to be generalized as part of your work, or at least avoid special-casing a ton of things, that's good.

I left more comments on the ClangImporter part, small as it was, but I don't think any of them are too significant.

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

Minor tweak, but looking really good.

@pschuh pschuh force-pushed the cpp-2 branch 2 times, most recently from cbb3710 to 3037d59 Compare July 27, 2019 00:21
@jrose-apple
Copy link
Contributor

I think I have no remaining comments; leaving it to @rjmccall for final approval.

@pschuh
Copy link
Contributor Author

pschuh commented Jul 29, 2019

@swift-ci please test

@pschuh
Copy link
Contributor Author

pschuh commented Jul 30, 2019

@swift-ci please test

@pschuh pschuh requested a review from rjmccall July 30, 2019 23:05
@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 3037d59196c3befebf7dfa6210061752d491b26d

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 3037d59196c3befebf7dfa6210061752d491b26d

@pschuh
Copy link
Contributor Author

pschuh commented Aug 1, 2019

@rjmccall John, could you take a quick look at this? It should be straightforward to approve.

@rjmccall
Copy link
Contributor

rjmccall commented Aug 1, 2019

I was just on vacation for three days; looking at it now.

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

LGTM

@rjmccall rjmccall merged commit 765d338 into swiftlang:master Aug 1, 2019
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