Skip to content
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.

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

Create anchors for pointer-based arguments to initializers #103

Merged
merged 5 commits into from
Apr 14, 2023

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented Apr 12, 2023

Fixes #102.

@arik-so arik-so force-pushed the init-pointer-anchoring branch from e53c45f to 6eae29b Compare April 12, 2023 22:58
@arik-so arik-so force-pushed the init-pointer-anchoring branch from 6eae29b to d3bcd0b Compare April 12, 2023 23:25
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

let returnValueHandlingPrefix = '';
let returnValueHandlingSuffix = '';
if (swiftMethodName === 'init') {
anchoringCommand = anchoringCommand.replaceAll('returnValue.addAnchor(', 'self.addAnchor(')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just define two separate commands instead of reusing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could, but I do like that this doesn't mess with the indentation.

@@ -807,6 +815,10 @@ export abstract class BaseTypeGenerator<Type extends RustType> {
}
`;

if(memoryContext && memoryContext.isConstructor && (argument.type instanceof RustStruct || argument.type instanceof RustTaggedValueEnum || argument.type instanceof RustResult)){
Copy link
Collaborator

@tnull tnull Apr 13, 2023

Choose a reason for hiding this comment

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

Could you add a brief comment here explaining what anchoring is and why we it is required in these cases in particular?

@@ -1284,6 +1296,11 @@ export interface PreparedArgument {
* memory deallocation or, ironically, retention beyond the call site
*/
deferredCleanup: string

/**
* If true (only applicable in constructors), add a `self.addAnchor` call after super.init()
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC there are however other cases were the anchors are added, not only upon init?

@@ -301,9 +302,13 @@ public class HumanObjectPeerTestInstance {
// channel manager constructor is mandatory

let graph = NetworkGraph(network: .Regtest, logger: self.logger)
var scorerGraph = graph
if(master.configuration.ephemeralNetworkGraphForScorer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Could add some space after if and drop the parentheses.

@@ -465,6 +465,9 @@ class LDKSwiftTests: XCTestCase {

config.useRouter = (i & (1 << 2)) != 0
print("useRouter: \(config.useRouter)")

config.ephemeralNetworkGraphForScorer = (i & (1 << 3)) != 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: May want to add a comment on how these magic shiftings work.

@@ -465,6 +465,9 @@ class LDKSwiftTests: XCTestCase {

config.useRouter = (i & (1 << 2)) != 0
print("useRouter: \(config.useRouter)")

config.ephemeralNetworkGraphForScorer = (i & (1 << 3)) != 0
print("ephemeralNetworkGraphForScorer: \(config.ephemeralNetworkGraphForScorer)")

/*
Copy link
Collaborator

@tnull tnull Apr 13, 2023

Choose a reason for hiding this comment

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

nit: Can we drop the commented-out code here and above while we're here?

@arik-so arik-so merged commit b8f3009 into lightningdevkit:main Apr 14, 2023
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.

Add anchors for objects if they're pointer arguments in an initializer
2 participants