Skip to content

bridging: reduce #ifdef USED_IN_CPP_SOURCE in bridging headers #77185

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 25, 2024
Merged

bridging: reduce #ifdef USED_IN_CPP_SOURCE in bridging headers #77185

merged 1 commit into from
Oct 25, 2024

Conversation

eeckstein
Copy link
Contributor

Especially avoid any constructors in #ifdef USED_IN_CPP_SOURCE blocks, because this breaks Windows ARM64.

@eeckstein eeckstein requested review from hjyamauchi and removed request for xedin, slavapestov, jckarter, DougGregor, bnbarham, CodaFi, ahoppen and hborla October 23, 2024 11:34
@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein
Copy link
Contributor Author

@hjyamauchi Can you please check that this change doesn't break anything on Windows ARM64? It should not, because it should make it even more robust.

@hjyamauchi
Copy link
Contributor

@eeckstein Sure thing.

I have been locally testing the Windows ARM64 compiler with this patch: #77193 since October 1 or so.

I will test the Windows ARM64 compiler with this change (#77185) and rebase #77193 after it and send it for review, if necessary.

Thanks for taking care of this.

@hjyamauchi
Copy link
Contributor

@swift-ci please test macOS Platform

@hjyamauchi
Copy link
Contributor

@eeckstein My testing of the ARM64 compiler is a bit behind in terms of the git history where I've so far validated up to Oct 2 2024 or so (I'm working my way up toward tip of tree) and this PR doesn't cleanly apply at that point. It'd need to take me a while to test this patch. Alternatively, you are welcome to merge this now and I will follow up with testing later and a change if necessary.

As you probably understand, the rule of thumb is (1) never put a constructor inside #ifdef _USED_IN_CPP_SOURCE...#endif or (2) match the condition of whether there is at least one user-defined constructor between when _USED_IN_CPP_SOURCE=1 and when _USED_IN_CPP_SOURCE=0 in any of the Bridged C++ class/structs. This PR looks good to me.

@hjyamauchi
Copy link
Contributor

Oh, one thing I just noticed: USED_IN_CPP_SOURC is a typo? Probably doesn't matter?

swift/include/swift/Parse/ParseBridging.h

Line 19 in 132726c

#ifdef USED_IN_CPP_SOURC 

#ifdef USED_IN_CPP_SOURC
#include "swift/Parse/Parser.h"
#else
namespace swift {
class Parser;
}
#endif

@eeckstein
Copy link
Contributor Author

USED_IN_CPP_SOURC is a typo?

Yes, thanks! This is not needed at all. I cleaned up the header includes in the bridging files a bit.

@eeckstein
Copy link
Contributor Author

@swift-ci test

hamishknight
hamishknight approved these changes Oct 24, 2024
View reviewed changes
include/swift/AST/ASTBridgingImpl.h
Comment on lines +160 to +164
BridgedDiagnosticArgument::BridgedDiagnosticArgument(const swift::DiagnosticArgument &arg) {
*reinterpret_cast<swift::DiagnosticArgument *>(&storage) = arg;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Looks like this needs clang-format'ing

@eeckstein
bridging: reduce #ifdef USED_IN_CPP_SOURCE in bridging headers
ed67e36 
Especially avoid any constructors in `#ifdef USED_IN_CPP_SOURCE` blocks, because this breaks Windows ARM64.
@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein eeckstein merged commit c429766 into swiftlang:main Oct 25, 2024
5 checks passed
@eeckstein eeckstein deleted the improve-bridging branch October 25, 2024 15:51
@hjyamauchi
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamishknight hamishknight hamishknight approved these changes

@hjyamauchi hjyamauchi hjyamauchi approved these changes

@rintaro rintaro Awaiting requested review from rintaro rintaro is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eeckstein @hjyamauchi @hamishknight