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.

Sign up for GitHub

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

Jump to bottom

[Interop][SwiftToCxx] Implement enum case switching #59744

Merged
merged 4 commits into from Jul 13, 2022
Merged

[Interop][SwiftToCxx] Implement enum case switching #59744

merged 4 commits into from Jul 13, 2022

Conversation

WANGJIEKE
Copy link
Contributor

This is a follow-up of pull request #59669. This pull request implements operator cases() and various predicates.

For example, following Swift enum

public enum Foo { case x, y }

will have member functions in the generated C++ header file like this

class Foo final {
public:
  enum class cases {
    x,
    y
  };
  operator cases() const {
    switch (_getEnumTag()) {
      case 0: return cases::x;
      case 1: default: return cases::y;
    }
  }
  bool isX() const {
    return *this == cases::x;
  }
  bool isY() const {
    return *this == cases::y;
  }
};

Other changes:

  • Print EnumValueWitnessTable struct when printing C++ interop scaffold;
  • Add _getEnumTag helper function for retrieving the tag from enum value witness table.

@WANGJIEKE
Copy link
Contributor Author

@hyp Could you please review this PR? Thanks!

I have a question regarding naming predicate functions: is std::toupper a good way to capitalize a letter? I'm worrying if there will be some corner cases I'm unaware of.

@hyp hyp added the c++ interop Feature: Interoperability with C++ label Jun 28, 2022
@hyp
Copy link
Contributor

hyp commented Jun 28, 2022

@swift-ci please test

@WANGJIEKE WANGJIEKE marked this pull request as ready for review June 28, 2022 20:03
hyp
hyp reviewed Jun 29, 2022
View reviewed changes
test/Interop/SwiftToCxx/enums/enum-cxx-clike-enum-case-functions-execution.cpp Outdated
// RUN: %target-codesign %t/swift-enums-execution
// RUN: %target-run %t/swift-enums-execution

// REQUIRES: executable_test
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also disable it on arm64e : ‘// UNSUPPORTED: CPU=arm64e’

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah I forgot this.

hyp
hyp reviewed Jun 29, 2022
View reviewed changes
lib/PrintAsClang/DeclAndTypePrinter.cpp Outdated
if (i != elements.size() - 1) {
os << " case " << i << ": return cases::";
} else {
os << " case " << i << ": default: return cases::";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a default case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to silence the non-exhaustive switch warning from the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

For sure. Unfortunately I don’t think the default case should map to a specific case. I think by default it should abort as that’s an unexpected path for fixed enums. Resilient enums are more complicated as they could have an unknown case, but we can handle them in a follow up.

hyp
hyp reviewed Jun 29, 2022
View reviewed changes
lib/PrintAsClang/DeclAndTypePrinter.cpp Outdated
}

// Rearrange cases so that they match their tag values
llvm::stable_sort(elements, [&](const auto *e1, const auto *e2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you determine that this is the correct ordering? Is the compiler doing this kind of sort anywhere else? We might want to reuse this if thins is already being done elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it from the memory layout doc, but yeah it's better to reuse existing functionality. I'll ask this in the office hour meeting.

@hyp
Copy link
Contributor

hyp commented Jun 29, 2022

Can you also add check lines for implementation of ‘_getEnumTag’

@hyp
Copy link
Contributor

hyp commented Jun 29, 2022

@hyp Could you please review this PR? Thanks!

I have a question regarding naming predicate functions: is std::toupper a good way to capitalize a letter? I'm worrying if there will be some corner cases I'm unaware of.

For now it’s fine, we already use it for properties. It might not handle Unicode correctly, but we will refactor and fix name transformations later.

@WANGJIEKE WANGJIEKE marked this pull request as draft June 29, 2022 11:09
@WANGJIEKE
Copy link
Contributor Author

@egorzhdan Could you please help me invoke the testing? Thanks!

@egorzhdan
Copy link
Collaborator

@swift-ci please test

@WANGJIEKE WANGJIEKE force-pushed the cxx-interop-switch-enum-impl branch from 029dc2f to 7ebcb7b Compare July 1, 2022 05:42
@WANGJIEKE WANGJIEKE marked this pull request as ready for review July 1, 2022 05:44
@hyp
Copy link
Contributor

hyp commented Jul 5, 2022

@swift-ci please test

@hyp
Copy link
Contributor

hyp commented Jul 5, 2022

Looks good apart from one request

@hyp
Copy link
Contributor

hyp commented Jul 6, 2022

@swift-ci please test

@hyp
Copy link
Contributor

hyp commented Jul 6, 2022

@swift-ci please test source compatibility

@hyp
Copy link
Contributor

hyp commented Jul 6, 2022

@swift-ci please test windows platform

@WANGJIEKE
Copy link
Contributor Author

Also change the return type of IRABIDetailsProvider::getEnumTagMapping from std::map to llvm::MapVector so that it actually returns the cases in their declaration order.

@hyp
Copy link
Contributor

hyp commented Jul 7, 2022

@swift-ci please test

@hyp
Copy link
Contributor

hyp commented Jul 7, 2022

@swift-ci please test source compatibility

@hyp
Copy link
Contributor

hyp commented Jul 8, 2022

@swift-ci please test

hyp
hyp approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@hyp hyp left a comment

Choose a reason for hiding this comment

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

LGTM

@hyp hyp merged commit 1821b62 into apple:main Jul 13, 2022
@WANGJIEKE WANGJIEKE deleted the cxx-interop-switch-enum-impl branch July 13, 2022 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyp hyp hyp approved these changes

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@WANGJIEKE @hyp @egorzhdan