[cxx-interop] Generate IR for decls called from members

[scentini]

Currently the following code doesn't work when callConstructor() is called from Swift:

inline int increment(int value) {
  return value + 1;
}

struct Incrementor {
  int incrementee;
  Incrementor(int value) : incrementee(increment(value)) {}
}

int callConstructor(int value) {
  return Incrementor(value).incrementee;
}

The issue is that we don't generate IR for the increment() function when it's only called from a constructor or a method.
Swift is aware of the existence of increment() and we see it in IR as declare incrementEi, however, as we don't to emit a definition, we get the following error:

Incrementor::Incrementor(int): error: undefined reference to 'increment(int)'

This PR fixes this by visiting constructors and methods in IRGen and calling HandleTopLevelDecl() with all used declarations, which results in emitting definitions for the used declarations.

[scentini]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 6559d45

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 6559d45

[scentini]

@swift-ci please test

[scentini]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - ba25b61

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - ba25b61

[zoecarver]

LGTM now. Thanks.

[scentini]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 54e220f

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 54e220f

[scentini]

@swift-ci please test

[zoecarver]

Does Cxx/members already exist? If not, can we name it member (no "s") so that it's more in line with the other test directories?

Also, any of the callConstructor tests should go in the /class directory. And arguable so should all the tests.

[zoecarver]

Also, is there any way we combine some of these tests? I don't see any reason we need half-a-dozen new headers and a dozen new test files. It will make things more difficult to find in the future.

Maybe you could add some of these to the existing member/ctor test files/headers.

[gribozavr]

I think this is a quite different scenario from existing constructor tests. Existing constructor tests test constructors in isolation. Here we have an interaction between three features: constructors, inline definitions of constructors, and inline functions. I think putting these tests (effectively integration tests) into separate files is the right choice.

[scentini]

I agree that 'members' is not a good name for these tests but I failed to find a more suitable name and I'm happy to change it if a better one comes to mind. What these tests to is check that IR is generated for inline functions when called from constructors/members. I'd like to keep them separate from the rest of the tests on constructors and members for the reason @gribozavr mentioned above.

[zoecarver]

I think this is a quite different scenario from existing constructor tests. Existing constructor tests test constructors in isolation. Here we have an interaction between three features: constructors, inline definitions of constructors, and inline functions. I think putting these tests (effectively integration tests) into separate files is the right choice.

Sorry if I was unclear, I'm not suggesting that we merge these with the existing constructor tests. I'm saying that there are about half-a-dozen header files in this patch that all essentially test "a constructor that calls an inline function." I think all (or at least some) of these headers could be merged into one header. Then instead of having ~six headers that all test "a constructor that calls an inline function" via six different classes all called "Incrementor" we would have "inline-function-in-ctor.h" which would have classes called "CallsInlineFunction", "CallFunctionFromNestedStruct", etc.

Either way, I think we should still probably put these tests in Cxx/class, as that is where the existing ctor/member tests are. At the very least, Cxx/members should be renamed to Cxx/member (no "s") to be in line with the rest of the C++ interop tests.

[scentini]

I moved the tests into Cxx/class/inline-functions-through-member, as to keep them separate from future tests on members.

As for the headers - I am strongly in favor of keeping tests as isolated as possible.
Putting everything in one header will reduce readability when one looks at a single test (eg. me 6 months from now). And the similarity between the content of the header files doesn't make things better - no two headers call the increment function from the same place, so we'd need a constructor that calls the function, a method that calls the function, a nested struct with a constructor that calls the function etc. - the sum of all these is less readable than having them in separate headers.

[zoecarver]

I moved the tests into Cxx/class/inline-functions-through-member, as to keep them separate from future tests on members.

Fantastic. I really like using a subdirectory here.

As for the headers - I am strongly in favor of keeping tests as isolated as possible.
Putting everything in one header will reduce readability when one looks at a single test (eg. me 6 months from now). And the similarity between the content of the header files doesn't make things better - no two headers call the increment function from the same place, so we'd need a constructor that calls the function, a method that calls the function, a nested struct with a constructor that calls the function etc. - the sum of all these is less readable than having them in separate headers.

Fair enough.

[gribozavr]

I think this is a quite different scenario from existing constructor tests. Existing constructor tests test constructors in isolation. Here we have an interaction between three features: constructors, inline definitions of constructors, and inline functions. I think putting these tests (effectively integration tests) into separate files is the right choice.

[scentini]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 8bcef1b

[zoecarver]

Can you file an SR for this and reference it here?

[zoecarver]

Oops, I see I already made this comment. Sorry. Anyway, can you please update the comments to include the created SR? Thanks!

[scentini]

I did that in 8bcef1b :)

[zoecarver]

Thanks.

[scentini]

@swift-ci please test