Skip to content

Commit 528cbab

Browse files
madsodgaardmadsodgaard
committed
PR feedback
1 parent a6c2fb9 commit 528cbab

File tree

5 files changed

+98
-50
lines changed

5 files changed

+98
-50
lines changed

‎Samples/JExtractJNISampleApp/Sources/MySwiftLibrary/Optionals.swift‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,3 +51,15 @@ public func optionalString(input: Optional<String>) -> String? {
5151
public func optionalClass(input: Optional<MySwiftClass>) -> MySwiftClass? {
5252
return input
5353
}
54+
55+
public func multipleOptionals(
56+
input1: Optional<Int8>,
57+
input2: Optional<Int16>,
58+
input3: Optional<Int32>,
59+
input4: Optional<Int64>,
60+
input5: Optional<String>,
61+
input6: Optional<MySwiftClass>,
62+
input7: Optional<Bool>
63+
) -> Int64? {
64+
return 1
65+
}

‎Samples/JExtractJNISampleApp/src/test/java/com/example/swift/OptionalsTest.java‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//
33
// This source file is part of the Swift.org open source project
44
//
5-
// Copyright (c) 2024 Apple Inc. and the Swift.org project authors
5+
// Copyright (c) 2025 Apple Inc. and the Swift.org project authors
66
// Licensed under Apache License v2.0
77
//
88
// See LICENSE.txt for license information
@@ -90,4 +90,21 @@ void optionalClass() {
9090
assertEquals(c.getX(), optionalClass.get().getX());
9191
}
9292
}
93+
94+
@Test
95+
void multipleOptionals() {
96+
try (var arena = new ConfinedSwiftMemorySession()) {
97+
MySwiftClass c = MySwiftClass.init(arena);
98+
OptionalLong result = MySwiftLibrary.multipleOptionals(
99+
Optional.of((byte) 1),
100+
Optional.of((short) 42),
101+
OptionalInt.of(50),
102+
OptionalLong.of(1000L),
103+
Optional.of("42"),
104+
Optional.of(c),
105+
Optional.of(true)
106+
);
107+
assertEquals(result, OptionalLong.of(1L));
108+
}
109+
}
93110
}

‎Sources/JExtractSwiftLib/Convenience/JavaType+Extensions.swift‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,52 @@ extension JavaType {
3838
}
3939

4040
/// Returns the next integral type with space for self and an additional byte.
41-
var nextIntergralTypeWithSpaceForByte: (java: JavaType, swift: String, valueBytes: Int)? {
41+
var nextIntergralTypeWithSpaceForByte: (javaType: JavaType, swiftType: SwiftKnownTypeDeclKind, valueBytes: Int)? {
4242
switch self {
43-
case .boolean, .byte: (.short, "Int16", 1)
44-
case .char, .short: (.int, "Int32", 2)
45-
case .int: (.long, "Int64", 4)
43+
case .boolean, .byte: (.short, .int16, 1)
44+
case .char, .short: (.int, .int32, 2)
45+
case .int: (.long, .int64, 4)
4646
default: nil
4747
}
4848
}
49+
50+
var optionalType: String? {
51+
switch self {
52+
case .boolean: "Optional<Boolean>"
53+
case .byte: "Optional<Byte>"
54+
case .char: "Optional<Character>"
55+
case .short: "Optional<Short>"
56+
case .int: "OptionalInt"
57+
case .long: "OptionalLong"
58+
case .float: "Optional<Float>"
59+
case .double: "OptionalDouble"
60+
case .javaLangString: "Optional<String>"
61+
default: nil
62+
}
63+
}
64+
65+
var optionalWrapperType: String? {
66+
switch self {
67+
case .boolean, .byte, .char, .short, .float, .javaLangString: "Optional"
68+
case .int: "OptionalInt"
69+
case .long: "OptionalLong"
70+
case .double: "OptionalDouble"
71+
default: nil
72+
}
73+
}
74+
75+
var optionalPlaceholderValue: String? {
76+
switch self {
77+
case .boolean: "false"
78+
case .byte: "(byte) 0"
79+
case .char: "(char) 0"
80+
case .short: "(short) 0"
81+
case .int: "0"
82+
case .long: "0L"
83+
case .float: "0f"
84+
case .double: "0.0"
85+
case .array, .class: "null"
86+
case .void: nil
87+
}
88+
}
4989
}

‎Sources/JExtractSwiftLib/JNI/JNISwift2JavaGenerator+JavaTranslation.swift‎

Lines changed: 6 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -289,17 +289,7 @@ extension JNISwift2JavaGenerator {
289289
throw JavaTranslationError.unsupportedSwiftType(swiftType)
290290
}
291291

292-
let (translatedClass, orElseValue) = switch javaType {
293-
case .boolean: ("Optional<Boolean>", "false")
294-
case .byte: ("Optional<Byte>", "(byte) 0")
295-
case .char: ("Optional<Character>", "(char) 0")
296-
case .short: ("Optional<Short>", "(short) 0")
297-
case .int: ("OptionalInt", "0")
298-
case .long: ("OptionalLong", "0L")
299-
case .float: ("Optional<Float>", "0f")
300-
case .double: ("OptionalDouble", "0.0")
301-
case .javaLangString: ("Optional<String>", #""""#)
302-
default:
292+
guard let translatedClass = javaType.optionalType, let placeholderValue = javaType.optionalPlaceholderValue else {
303293
throw JavaTranslationError.unsupportedSwiftType(swiftType)
304294
}
305295

@@ -311,7 +301,7 @@ extension JNISwift2JavaGenerator {
311301
),
312302
conversion: .commaSeparated([
313303
.isOptionalPresent,
314-
.method(.placeholder, function: "orElse", arguments: [.constant(orElseValue)])
304+
.method(.placeholder, function: "orElse", arguments: [.constant(placeholderValue)])
315305
])
316306
)
317307
}
@@ -406,30 +396,20 @@ extension JNISwift2JavaGenerator {
406396
throw JavaTranslationError.unsupportedSwiftType(swiftType)
407397
}
408398

409-
let (returnType, optionalClass) = switch javaType {
410-
case .boolean: ("Optional<Boolean>", "Optional")
411-
case .byte: ("Optional<Byte>", "Optional")
412-
case .char: ("Optional<Character>", "Optional")
413-
case .short: ("Optional<Short>", "Optional")
414-
case .int: ("OptionalInt", "OptionalInt")
415-
case .long: ("OptionalLong", "OptionalLong")
416-
case .float: ("Optional<Float>", "Optional")
417-
case .double: ("OptionalDouble", "OptionalDouble")
418-
case .javaLangString: ("Optional<String>", "Optional")
419-
default:
399+
guard let returnType = javaType.optionalType, let optionalClass = javaType.optionalWrapperType else {
420400
throw JavaTranslationError.unsupportedSwiftType(swiftType)
421401
}
422402

423403
// Check if we can fit the value and a discriminator byte in a primitive.
424-
// so the return JNI value will be (value || discriminator)
404+
// so the return JNI value will be (value, discriminator)
425405
if let nextIntergralTypeWithSpaceForByte = javaType.nextIntergralTypeWithSpaceForByte {
426406
return TranslatedResult(
427407
javaType: .class(package: nil, name: returnType),
428408
annotations: parameterAnnotations,
429409
outParameters: [],
430410
conversion: .combinedValueToOptional(
431411
.placeholder,
432-
nextIntergralTypeWithSpaceForByte.java,
412+
nextIntergralTypeWithSpaceForByte.javaType,
433413
valueType: javaType,
434414
valueSizeInBytes: nextIntergralTypeWithSpaceForByte.valueBytes,
435415
optionalType: optionalClass
@@ -578,9 +558,6 @@ extension JNISwift2JavaGenerator {
578558

579559
case constant(String)
580560

581-
// The input exploded into components.
582-
case explodedName(component: String)
583-
584561
// Convert the results of the inner steps to a comma separated list.
585562
indirect case commaSeparated([JavaNativeConversionStep])
586563

@@ -640,9 +617,6 @@ extension JNISwift2JavaGenerator {
640617
case .constant(let value):
641618
return value
642619

643-
case .explodedName(let component):
644-
return "\(placeholder)_\(component)"
645-
646620
case .commaSeparated(let list):
647621
return list.map({ $0.render(&printer, placeholder)}).joined(separator: ", ")
648622

@@ -713,7 +687,7 @@ extension JNISwift2JavaGenerator {
713687
/// Whether the conversion uses SwiftArena.
714688
var requiresSwiftArena: Bool {
715689
switch self {
716-
case .placeholder, .constant, .explodedName, .isOptionalPresent:
690+
case .placeholder, .constant, .isOptionalPresent:
717691
return false
718692

719693
case .constructSwiftValue:

‎Sources/JExtractSwiftLib/JNI/JNISwift2JavaGenerator+NativeTranslation.swift‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ extension JNISwift2JavaGenerator {
165165
wrappedType swiftType: SwiftType,
166166
parameterName: String
167167
) throws -> NativeParameter {
168-
let descriptorParameter = JavaParameter(name: "\(parameterName)_descriptor", type: .byte)
168+
let discriminatorName = "\(parameterName)_discriminator"
169169
let valueName = "\(parameterName)_value"
170170

171171
switch swiftType {
@@ -178,10 +178,14 @@ extension JNISwift2JavaGenerator {
178178

179179
return NativeParameter(
180180
parameters: [
181-
descriptorParameter,
181+
JavaParameter(name: discriminatorName, type: .byte),
182182
JavaParameter(name: valueName, type: javaType)
183183
],
184-
conversion: .optionalLowering(.initFromJNI(.placeholder, swiftType: swiftType))
184+
conversion: .optionalLowering(
185+
.initFromJNI(.placeholder, swiftType: swiftType),
186+
discriminatorName: discriminatorName,
187+
valueName: valueName
188+
)
185189
)
186190
}
187191

@@ -220,15 +224,15 @@ extension JNISwift2JavaGenerator {
220224
}
221225

222226
// Check if we can fit the value and a discriminator byte in a primitive.
223-
// so the return JNI value will be (value || discriminator)
227+
// so the return JNI value will be (value, discriminator)
224228
if let nextIntergralTypeWithSpaceForByte = javaType.nextIntergralTypeWithSpaceForByte {
225229
return NativeResult(
226-
javaType: nextIntergralTypeWithSpaceForByte.java,
230+
javaType: nextIntergralTypeWithSpaceForByte.javaType,
227231
conversion: .getJNIValue(
228232
.optionalRaisingWidenIntegerType(
229233
.placeholder,
230234
valueType: javaType,
231-
combinedSwiftType: nextIntergralTypeWithSpaceForByte.swift,
235+
combinedSwiftType: nextIntergralTypeWithSpaceForByte.swiftType,
232236
valueSizeInBytes: nextIntergralTypeWithSpaceForByte.valueBytes
233237
)
234238
),
@@ -454,11 +458,11 @@ extension JNISwift2JavaGenerator {
454458

455459
case initializeJavaKitWrapper(wrapperName: String)
456460

457-
indirect case optionalLowering(NativeSwiftConversionStep)
461+
indirect case optionalLowering(NativeSwiftConversionStep, discriminatorName: String, valueName: String)
458462

459463
indirect case optionalChain(NativeSwiftConversionStep)
460464

461-
indirect case optionalRaisingWidenIntegerType(NativeSwiftConversionStep, valueType: JavaType, combinedSwiftType: String, valueSizeInBytes: Int)
465+
indirect case optionalRaisingWidenIntegerType(NativeSwiftConversionStep, valueType: JavaType, combinedSwiftType: SwiftKnownTypeDeclKind, valueSizeInBytes: Int)
462466

463467
indirect case optionalRaisingIndirectReturn(NativeSwiftConversionStep, returnType: JavaType, discriminatorParameterName: String, placeholderValue: NativeSwiftConversionStep)
464468

@@ -571,9 +575,9 @@ extension JNISwift2JavaGenerator {
571575
case .initializeJavaKitWrapper(let wrapperName):
572576
return "\(wrapperName)(javaThis: \(placeholder), environment: environment!)"
573577

574-
case .optionalLowering(let valueConversion):
575-
let value = valueConversion.render(&printer, "\(placeholder)_value")
576-
return "\(placeholder)_descriptor == 1 ? \(value) : nil"
578+
case .optionalLowering(let valueConversion, let discriminatorName, let valueName):
579+
let value = valueConversion.render(&printer, valueName)
580+
return "\(discriminatorName) == 1 ? \(value) : nil"
577581

578582
case .optionalChain(let inner):
579583
let inner = inner.render(&printer, placeholder)
@@ -582,10 +586,11 @@ extension JNISwift2JavaGenerator {
582586
case .optionalRaisingWidenIntegerType(let inner, let valueType, let combinedSwiftType, let valueSizeInBytes):
583587
let inner = inner.render(&printer, placeholder)
584588
let value = valueType == .boolean ? "$0 ? 1 : 0" : "$0"
589+
let combinedSwiftTypeName = combinedSwiftType.moduleAndName.name
585590
printer.print(
586591
"""
587-
let value$: \(combinedSwiftType) = \(inner).map {
588-
\(combinedSwiftType)(\(value)) << \(valueSizeInBytes * 8) | \(combinedSwiftType)(1)
592+
let value$ = \(inner).map {
593+
\(combinedSwiftTypeName)(\(value)) << \(valueSizeInBytes * 8) | \(combinedSwiftTypeName)(1)
589594
} ?? 0
590595
"""
591596
)

0 commit comments

Comments
 (0)