Skip to content

[cxx-interop] Support bool-based enums.#34365

Merged
swift-ci merged 1 commit intoswiftlang:mainfrom
zoecarver:cxx/bool-enum
Oct 26, 2020
Merged

[cxx-interop] Support bool-based enums.#34365
swift-ci merged 1 commit intoswiftlang:mainfrom
zoecarver:cxx/bool-enum

Conversation

@zoecarver
Copy link
Contributor

@zoecarver zoecarver commented Oct 20, 2020

This is a small fix to prevent a crash. This change simply adds another condition for the bool branch that checks if the APInt has a bit-width of 1.

There are some big changes we should make in the future around enums. But, this patch is only to prevent a crash.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 20, 2020

@swift-ci please smoke test.

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Oct 20, 2020
varungandhi-apple
varungandhi-apple reviewed Oct 21, 2020
View reviewed changes
@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 22, 2020

@swift-ci please smoke test and merge.

1 similar comment
@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 22, 2020

@swift-ci please smoke test and merge.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 22, 2020

@swift-ci please test Windows.

1 similar comment
@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 22, 2020

@swift-ci please test Windows.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 22, 2020 •
edited
Loading

It looks like I'll need to switch back to my original implementation. Sometimes a user of createConstant is trying to get a Bool type but doesn't pass an APInt with width 1. (The Interop tests didn't pick this up but the IDE and ClangImporter tests did.)

@varungandhi-apple
Copy link
Contributor

varungandhi-apple commented Oct 22, 2020

If that's the case, please add a comment indicating why the APInt condition doesn't imply the Bool condition. Otherwise, someone reading the code later might have the same question that I had.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 25, 2020

@varungandhi-apple I've updated createConstant to check if type is associated with a clang::EnumDecl and if that enum decl's base type is _Bool then set isBool = true. I think this is more correct because there's no guarantee that the bit-width will be 1 if the base type is a boolean. What do you think?

@zoecarver
[cxx-interop] Support bool-based enums.
05faa07 
This is a small fix to prevent a crash. This change simply adds another
condition for the "bool" branch that checks if "type" is associated with
a "clang::EnumDecl" with an underlying type of "bool", and if so, treats
"type" as a "Bool".
@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 25, 2020

@swift-ci please smoke test.

4 similar comments
@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 25, 2020

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 25, 2020

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 25, 2020

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 26, 2020

@swift-ci please smoke test.

varungandhi-apple
varungandhi-apple reviewed Oct 26, 2020
View reviewed changes
lib/ClangImporter/ImportDecl.cpp
Comment on lines +8564 to +8566
type->getStructOrBoundGenericStruct()->getClangDecl()) {
if (auto enumDecl = dyn_cast<clang::EnumDecl>(
type->getStructOrBoundGenericStruct()->getClangDecl())) {
Copy link
Contributor

@varungandhi-apple varungandhi-apple Oct 26, 2020

Choose a reason for hiding this comment

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

You could simplify this a little bit by using dyn_cast_or_null.

varungandhi-apple
varungandhi-apple approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@varungandhi-apple varungandhi-apple left a comment

Choose a reason for hiding this comment

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

Thanks. This code is a bit longer than what you had earlier, but it seems much more understandable.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 26, 2020

Testing again now that #34440 has landed.

@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 26, 2020

@swift-ci please smoke test and merge.

1 similar comment
@zoecarver
Copy link
Contributor Author

zoecarver commented Oct 26, 2020

@swift-ci please smoke test and merge.

@swift-ci swift-ci merged commit 26c9626 into swiftlang:main Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rjmccall rjmccall rjmccall approved these changes

@gribozavr gribozavr Awaiting requested review from gribozavr

@MForster MForster Awaiting requested review from MForster

+1 more reviewer

@varungandhi-apple varungandhi-apple varungandhi-apple approved these changes

Reviewers whose approvals may not affect merge requirements

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.

4 participants

@zoecarver @varungandhi-apple @rjmccall @swift-ci