Skip to content

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

Merged
hyp merged 4 commits intoswiftlang:mainfrom
WANGJIEKE:cxx-interop-switch-enum-impl
Jul 13, 2022
Merged

[Interop][SwiftToCxx] Implement enum case switching#59744
hyp merged 4 commits intoswiftlang:mainfrom
WANGJIEKE:cxx-interop-switch-enum-impl

Conversation

@WANGJIEKE
Copy link
Copy Markdown
Contributor

@WANGJIEKE WANGJIEKE commented Jun 28, 2022

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
Copy Markdown
Contributor Author

WANGJIEKE commented Jun 28, 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.

@hyp hyp added the c++ interop Feature: Interoperability with C++ label Jun 28, 2022
@hyp
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor

@hyp hyp Jun 29, 2022

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
Copy Markdown
Contributor Author

@WANGJIEKE WANGJIEKE Jun 29, 2022

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
Copy link
Copy Markdown
Contributor

@hyp hyp Jun 29, 2022

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
Copy Markdown
Contributor Author

@WANGJIEKE WANGJIEKE Jun 29, 2022

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
Copy Markdown
Contributor

@hyp hyp Jul 5, 2022

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
Copy link
Copy Markdown
Contributor

@hyp hyp Jun 29, 2022

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
Copy Markdown
Contributor Author

@WANGJIEKE WANGJIEKE Jun 29, 2022

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
Copy Markdown
Contributor

hyp commented Jun 29, 2022

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

@hyp
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

WANGJIEKE commented Jun 30, 2022

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

@egorzhdan
Copy link
Copy Markdown
Contributor

egorzhdan commented Jun 30, 2022

@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
Copy Markdown
Contributor

hyp commented Jul 5, 2022

@swift-ci please test

@hyp
Copy link
Copy Markdown
Contributor

hyp commented Jul 5, 2022

Looks good apart from one request

@hyp
Copy link
Copy Markdown
Contributor

hyp commented Jul 6, 2022

@swift-ci please test

@hyp
Copy link
Copy Markdown
Contributor

hyp commented Jul 6, 2022

@swift-ci please test source compatibility

@hyp
Copy link
Copy Markdown
Contributor

hyp commented Jul 6, 2022

@swift-ci please test windows platform

@WANGJIEKE
Copy link
Copy Markdown
Contributor Author

WANGJIEKE commented Jul 6, 2022

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
Copy Markdown
Contributor

hyp commented Jul 7, 2022

@swift-ci please test

@hyp
Copy link
Copy Markdown
Contributor

hyp commented Jul 7, 2022

@swift-ci please test source compatibility

@hyp
Copy link
Copy Markdown
Contributor

hyp commented Jul 8, 2022

@swift-ci please test

hyp
hyp approved these changes Jul 13, 2022
View reviewed changes
Copy link
Copy Markdown
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 swiftlang: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.

3 participants

@WANGJIEKE @hyp @egorzhdan