Skip to content

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

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
Merged
merged 1 commit into from
Oct 26, 2020
Merged

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

merged 1 commit into from
Oct 26, 2020

Conversation

zoecarver
Copy link
Contributor

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

@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

@swift-ci please smoke test and merge.

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test and merge.

@zoecarver
Copy link
Contributor Author

@swift-ci please test Windows.

1 similar comment
@zoecarver
Copy link
Contributor Author

@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

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

@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

@swift-ci please smoke test.

4 similar comments
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@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

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

Testing again now that #34440 has landed.

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test and merge.

1 similar comment
@zoecarver
Copy link
Contributor Author

@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

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

@gribozavr gribozavr Awaiting requested review from gribozavr

@MForster MForster Awaiting requested review from MForster

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