Skip to content

Conversation

@NuriAmari
Copy link
Contributor

@NuriAmari NuriAmari commented Nov 18, 2022

Second attempt at: 62058

@NuriAmari
Copy link
Contributor Author

@swift-ci please smoke test

@NuriAmari NuriAmari changed the title Correct effective context translation for NS_OPTIONS anon C++ enums Correct effective context translation for NS_OPTIONS anon C++ enums attempt 2 Nov 18, 2022
@hyp
Copy link
Contributor

hyp commented Nov 18, 2022

@swift-ci please smoke test

Please run the full test suite instead of just smoke test .

@NuriAmari
Copy link
Contributor Author

Please run the full test suite instead of just smoke test .

Yes I will, I'm just trying to run the macOs smoke tests repeatedly since I'm unable to reproduce the failure that caused the revert locally, and it seems to have passed in many many CI runs prior to the revert. https://ci.swift.org/job/swift-PR-macos-smoke-test/4317/.

Did this break PR break something a full run of smoke test would've caught?

@NuriAmari
Copy link
Contributor Author

First smoke test:
image

@NuriAmari
Copy link
Contributor Author

@swift-ci please test

Prior to this patch, SwiftLookupTable::translateDeclToContext relied
on the `TypedefNameDeclOrQualifier` field of an anonymous tag decl to
create a name for entry representing an anonymous tag in the lookup
table. This field is not always populated by Clang, it is often
populated only for the purposes of generating a linkage name when the
type is introduced via typedef as follows:

```
typedef enum { option1, option2} MyAnonEnum;
```

The field is not populated for anonymous enums introduced by NS_OPTIONS
with cxx interop enabled. This patch adds a fallback check in
`translateDeclToContext` that if the field is empty, check if the enum
is backed by a typedef that is unavailable in Swift. If that is the
case, use that name for the lookup table entry.
@NuriAmari NuriAmari force-pushed the ns-option-lookup-table-warning-attempt-2 branch from 03bcf18 to 5f5bebf Compare November 18, 2022 19:12
@NuriAmari
Copy link
Contributor Author

@swift-ci please test

@@ -0,0 +1,11 @@
// RUN: %empty-directory(%t/cache)
Copy link
Contributor Author

@NuriAmari NuriAmari Nov 18, 2022

Choose a reason for hiding this comment

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

@hyp I think @drodriguez found the issue, these two tests were sharing a module cache and emptying it at the start of the test. If these tests are run concurrently you get weird flaky failures. I've changed %T -> %t to make sure the module cache directories are distinct.

@NuriAmari NuriAmari marked this pull request as ready for review November 20, 2022 17:26
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
Copy link
Contributor

hyp commented Nov 29, 2022

@swift-ci please test source compatibility

@NuriAmari NuriAmari merged commit b71f9d0 into swiftlang:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants