Remove CBasicBridging + CASTBridging #69440

hamishknight · 2023-10-26T20:06:41Z

Merge CASTBridging and CBasicBridging with ASTBridging and BasicBridging respectively, allowing ASTGen to use C++ interop, and for both ASTGen and SwiftCompilerSources to share the same bridging code. There's more work that can be done here to improve the bridging (e.g we could potentially remove the bridged and unbridged functions in ASTBridging.cpp now), but I'm leaving that for a future PR as there's already enough here.

lib/ASTGen/CMakeLists.txt

hamishknight · 2023-10-30T14:05:12Z

include/swift/AST/ASTBridgingWrappers.def

 #ifndef DECL
-#define DECL(Id, Parent) AST_BRIDGING_WRAPPER_NONNULL(Id##Decl)
+#define DECL(Id, Parent) AST_BRIDGING_WRAPPER_NULLABLE(Id##Decl)


At this point I kind of wonder whether we're really gaining much from defining separate macros for can-be-nullable, and never-nullable, and should just stamp out nullable bridging wrappers for everything. My main worry with that initially was that it would unnecessarily pollute the type namespace, but at this point a good chunk of types need nullable wrappers anyways. In any case, I can explore that in a follow up.

lib/ASTGen/Sources/ASTGen/ASTGen.swift

SwiftCompilerSources/Package.swift

bnbarham · 2023-10-30T16:38:24Z

include/swift/AST/ASTBridging.h


 SWIFT_END_NULLABILITY_ANNOTATIONS

+#ifndef PURE_BRIDGING_MODE
+// In _not_ PURE_BRIDGING_MODE, briding functions are inlined and therefore


Suggested change

// In _not_ PURE_BRIDGING_MODE, briding functions are inlined and therefore

// In _not_ PURE_BRIDGING_MODE, bridging functions are inlined and therefore

But also, why is that?

Because the bridging functions can require C++ headers that we don't want to include in "pure bridging mode", since that can trigger the Windows and LLDB C++ interop issues. I can update the comment to explain this (FWIW I copied it from SILBridging.h)

include/swift/Basic/BasicBridging.h

bnbarham · 2023-10-30T16:42:36Z

include/swift/Basic/BasicBridging.h

+  const void *_Nullable data;
+  size_t numElements;


Suppose these should both start with an uppercase :(. Looks like other Bridged* are similar too.

Look again :) But yeah I should probably fix the others too

Hah, nice. How far apart was your push and my hitting comment 😅? Doesn't have to be in this PR FWIW.

Yeah I think I'll do this in a follow up

include/swift/Basic/BasicBridging.h

bnbarham · 2023-10-30T16:56:21Z

lib/ASTGen/Sources/ASTGen/Bridge.swift

    try withUTF8 { buffer in
-      try body(BridgedString(data: buffer.baseAddress, length: buffer.count))
+      try body(BridgedStringRef(data: buffer.baseAddress, count: buffer.count))
    }
  }
 }

 /// Allocate a copy of the given string as a null-terminated UTF-8 string.


Could/should uses of this use BridgedOwnedString now?

Yeah seems like it probably should

lib/ASTGen/Sources/ASTGen/Types.swift

bnbarham · 2023-10-30T17:19:47Z

lib/Sema/TypeCheckMacros.cpp

    assert(!externalDef.isError());
    swift_ASTGen_expandFreestandingMacro(
        &ctx.Diags, externalDef.opaqueHandle,
        static_cast<uint32_t>(externalDef.kind), discriminator->c_str(),
        getRawMacroRole(macroRole), astGenSourceFile,
        expansion->getSourceRange().Start.getOpaquePointerValue(),
        &evaluatedSourceOut);
-    if (!evaluatedSourceOut.data)
+    if (!evaluatedSourceOut.get().data())


Not a huge fan of get. unbridged would match the bridging .cpp.

Hmm yeah that seems fair enough

Ah, I remember why I named it this way now, it's because it matches all the wrappers in SILBridging.h. If we want to change to unbridged we probably ought to change those too. Not sure if you have any strong opinions on this @eeckstein?

no strong opinions. We can change it

eeckstein

nice!

include/swift/Basic/BasicBridging.h

eeckstein

lgtm!

hamishknight · 2023-10-30T19:34:08Z

Will address the rest of the feedback in a follow up

Tweak the compiler flags such that it can be built from within Xcode. This is only useful when editing, but I find it very handy.

Improve APIs for BridgedStringRef, BridgedOwnedString, and BridgedSourceLoc.

Migrate onto BridgedStringRef, and the BasicBridging equivalents of BridgedSourceLoc and BridgedArrayRef.

These are `void *` on the C side.

Introduce a macro that can stamp out wrapper classes for underlying C++ pointers, and use it to define BridgedDiagnosticEngine in ASTBridging. Then, migrate users of BridgedDiagEngine onto it.

And renaming OptionalBridgedVarDecl to BridgedNullableVarDecl for consistency with the existing nullable AST node wrappers.

Merge with BasicBridging and ASTBridging respectively. The changes here should be pretty uncontroversial, I tried to keep it to just moving code about.

These are now stamped out by the ASTBridgingWrappers.

Move these into ASTBridgingWrappers.def.

These aren't needed in C++

Add a `Bridged` prefix to match the name being exposed to Swift, and to match the other bridging functions. Also while here, use `SWIFT_NAME` for some bridging functions that were missing it.

Sort AST node bridging functions into Expr, Stmt, Decl, and TypeRepr, and tweak the headers to use `MARK`.

Remove the default constructor footgun present with the struct implementations, and sprinkle some `SWIFT_NAME` and bridging utilities to make them nicer to work with.

These are now empty, and it seems like generally we ought to prefer putting the bridging logic on the BridgedXXX wrappers.

hamishknight · 2023-10-30T23:50:24Z

@swift-ci please test

hamishknight · 2023-10-31T09:38:56Z

🚢

Fix the build after swiftlang#69440

hyp reviewed Oct 26, 2023

View reviewed changes

lib/ASTGen/CMakeLists.txt Outdated Show resolved Hide resolved

hamishknight force-pushed the double-plus branch 6 times, most recently from e572b4a to 57384f4 Compare October 30, 2023 13:59

hamishknight changed the title ~~[WIP] Remove CBasicBridging + CASTBridging~~ Remove CBasicBridging + CASTBridging Oct 30, 2023

hamishknight marked this pull request as ready for review October 30, 2023 14:01

hamishknight requested review from hborla, slavapestov, xedin, ahoppen, bnbarham, CodaFi, DougGregor, rintaro, zoecarver and eeckstein as code owners October 30, 2023 14:01

hamishknight commented Oct 30, 2023

View reviewed changes

lib/ASTGen/Sources/ASTGen/ASTGen.swift Outdated Show resolved Hide resolved

bnbarham reviewed Oct 30, 2023

View reviewed changes

eeckstein reviewed Oct 30, 2023

View reviewed changes

include/swift/Basic/BasicBridging.h Show resolved Hide resolved

eeckstein approved these changes Oct 30, 2023

View reviewed changes

xedin removed their request for review October 30, 2023 18:31

hamishknight force-pushed the double-plus branch from 7324c42 to 4602196 Compare October 30, 2023 19:33

hamishknight added 3 commits October 30, 2023 23:49

[SwiftCompilerSources] NFC: Allow building Package.swift

26bfe10

Tweak the compiler flags such that it can be built from within Xcode. This is only useful when editing, but I find it very handy.

[ASTGen] Enable C++ interop

e5442fe

[SwiftCompilerSources] NFC: Workaround a compiler bug

3e3fb58

hamishknight added 14 commits October 30, 2023 23:49

[Basic] Improve some bridging APIs

ce23252

Improve APIs for BridgedStringRef, BridgedOwnedString, and BridgedSourceLoc.

[ASTGen] Migrate onto some BasicBridging types

356cf31

Migrate onto BridgedStringRef, and the BasicBridging equivalents of BridgedSourceLoc and BridgedArrayRef.

[ASTGen] Fix a couple of @_cdecl param types

fe0ad60

These are `void *` on the C side.

[AST] Merge BridgedDiagnosticEngine + BridgedDiagEngine

fce1cb5

Introduce a macro that can stamp out wrapper classes for underlying C++ pointers, and use it to define BridgedDiagnosticEngine in ASTBridging. Then, migrate users of BridgedDiagEngine onto it.

[AST] Move BridgedNominalTypeDecl + BridgedVarDecl to ASTBridging

cae5d33

And renaming OptionalBridgedVarDecl to BridgedNullableVarDecl for consistency with the existing nullable AST node wrappers.

Remove CBasicBridging and CASTBridging

6afde8a

Merge with BasicBridging and ASTBridging respectively. The changes here should be pretty uncontroversial, I tried to keep it to just moving code about.

[AST] Remove BridgedVarDecl + BridgedNominalTypeDecl duplicates

2ac0860

These are now stamped out by the ASTBridgingWrappers.

[AST] Abstract away a couple of bridging wrappers

28b7fd7

Move these into ASTBridgingWrappers.def.

[AST] Remove redundant typedefs

25546d2

These aren't needed in C++

[AST] Use consistent naming for bridging functions

dc899c5

Add a `Bridged` prefix to match the name being exposed to Swift, and to match the other bridging functions. Also while here, use `SWIFT_NAME` for some bridging functions that were missing it.

[AST] Cleanup ASTBridging + BasicBridging

e0f57f8

Sort AST node bridging functions into Expr, Stmt, Decl, and TypeRepr, and tweak the headers to use `MARK`.

[Basic] Improve API of BridgedArrayRef + BridgedData

2a6d93f

Remove the default constructor footgun present with the struct implementations, and sprinkle some `SWIFT_NAME` and bridging utilities to make them nicer to work with.

Remove BridgingUtils.h headers

5853304

These are now empty, and it seems like generally we ought to prefer putting the bridging logic on the BridgedXXX wrappers.

Address review feedback

3cb74e9

hamishknight force-pushed the double-plus branch from 4602196 to 3cb74e9 Compare October 30, 2023 23:50

hamishknight merged commit d67dc0a into swiftlang:main Oct 31, 2023

hamishknight deleted the double-plus branch October 31, 2023 09:39

hamishknight mentioned this pull request Oct 31, 2023

A couple of bridging cleanups #69534

Merged

compnerd mentioned this pull request Oct 31, 2023

Revert "Remove CBasicBridging + CASTBridging" #69536

Closed

compnerd added a commit to compnerd/apple-swift that referenced this pull request Oct 31, 2023

build: guard against builds where SwiftSyntax may not be enabled

8354c86

Fix the build after swiftlang#69440

compnerd mentioned this pull request Oct 31, 2023

build: guard against builds where SwiftSyntax may not be enabled #69537

Merged

	// In _not_ PURE_BRIDGING_MODE, briding functions are inlined and therefore
	// In _not_ PURE_BRIDGING_MODE, bridging functions are inlined and therefore

Remove CBasicBridging + CASTBridging #69440

Remove CBasicBridging + CASTBridging #69440

Uh oh!

Conversation

hamishknight commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

hamishknight commented Oct 30, 2023

Uh oh!

hamishknight commented Oct 30, 2023

Uh oh!

hamishknight commented Oct 31, 2023

Uh oh!

Uh oh!

hamishknight commented Oct 26, 2023 •

edited

Loading