Update public interface #134
Conversation
| #include "OGBase.h" | ||
|
|
||
| typedef OG_OPTIONS(uint32_t, OGAttributeFlags) { | ||
| typedef OG_OPTIONS(uint8_t, OGAttributeFlags) { |
There was a problem hiding this comment.
This gets stored as an 8-bit field in AG::Node.
| struct { | ||
| OGTypeID type_id; | ||
| const void *witness_table; | ||
| } body_conformance; |
There was a problem hiding this comment.
@Kyle-Ye This doesn't appear in the swift section dump you provided, but I can see it being set in Ghidra's disassembly. I'm not sure if there is a difference in behaviour depending on version.
There was a problem hiding this comment.
Do we miss something in swift section? Maybe we can try to use Ghidra and align those logic. cc @Mx-Iris
There was a problem hiding this comment.
I'm now on macOS Sequoia and the Swift section is reporting the structure is 56 bytes, which would exclude this field. But compatibility tests are confirming that it is definitely set.
https://github.com/jcmosc/Compute/blob/updating/Tests/ComputeTests/Shared/Attribute/AttributeTests.swift#L39
It's possible that it's not included in the attribute type struct, but instead part of a wrapper struct returned from the closure passed to AGGraphInternAttributeType.
|
Any reason why this is still work in progress? If you do not mind, I can help rebase and merge it now. cc @jcmosc |
Kyle-Ye
force-pushed
the
update-public-interface
branch
from
db8f5a7 to
0825d44
Compare
jcmosc
force-pushed
the
update-public-interface
branch
from
0825d44 to
504745e
Compare
|
@Kyle-Ye I didn't see your comment yesterday! I would consider this ready to merge, though I realise it's become a big PR. Happy to break it up into smaller ones if you prefer. |
Merge this as it is or split it both seem fine to me. Let's solve the final conflict and get it merged 🚀. |
jcmosc
force-pushed
the
update-public-interface
branch
3 times, most recently
from
059fc91 to
024fde2
Compare
|
@Kyle-Ye rebased without conflicts :) |
|
The compatibility tests is failing since we have not updated AG yet. I'll create a PR to update it. |
Kyle-Ye
left a comment
Kyle-Ye
left a comment
There was a problem hiding this comment.
98% change is LGTM.
1% change is NIT: Some missing OG_SWIFT_NAME can be added or a later PR.
1% change needs more discussion here: mainly Subgraph.Flags and Flags. (We can also merge it as it is after the AG repo bump if you do not have time recently and discuss it later.)
There was a problem hiding this comment.
I think we should keep the API on AnyAttribute side and Attribute side in sync.
There was a problem hiding this comment.
I can't see the line number this comment is attached to in GitHub. I assume you're referring to source vs indirectSource?
I originally thought the accessor traps if the attribute is not indirect and wanted to call this behaviour out. But I realise now it actually just returns itself.
I'll revert it to source.
| @_silgen_name("OGSubgraphApply") | ||
| private func OGSubgraphApply( | ||
| _ graph: Subgraph, | ||
| flags: OGAttributeFlags, | ||
| callback: (AnyAttribute) -> Void | ||
| ) | ||
|
|
||
| @_silgen_name("OGSubgraphAddObserver") | ||
| private func OGSubgraphAddObserver( | ||
| _ graph: Subgraph, | ||
| observer: () -> Void | ||
| ) -> Int | ||
| // FIXME: migrate to use @_extern(c, "xx") in Swift 6 | ||
| extension Subgraph { | ||
| @_silgen_name("OGSubgraphApply") | ||
| private static func apply(_ graph: Subgraph, flags: Subgraph.Flags, callback: (AnyAttribute) -> Void) | ||
|
|
||
| @_silgen_name("OGSubgraphAddObserver") | ||
| private static func addObserver(_ graph: Subgraph, observer: () -> Void) -> Int | ||
| } |
There was a problem hiding this comment.
Any reason to change this? Does it related with https://forums.swift.org/t/understanding-calling-conventions-passing-closure-from-swift-to-c-and-calling-it-from-c/79995?
If so, please add some comments context here.
There was a problem hiding this comment.
At the time this was the only way I could get the parameters to line up between C++ and Swift. But I was using @_extern(c) instead of @_silgen_name. I will experiment with @_silgen_name to see if it works without changes.
| typedef OG_OPTIONS(uint8_t, OGAttributeFlags) { | ||
| OGAttributeFlagsNone = 0, | ||
| OGAttributeFlagsAll = 0xFF, | ||
| } OG_SWIFT_NAME(OGSubgraphRef.Flags); |
There was a problem hiding this comment.
Why we are removing the case for OGAttributeFlags and rename it to OGSubgraphRef.Flags
There was a problem hiding this comment.
From OG's point of view, these flags have no semantics. They function more like a userInfo dictionary. It would be in OpenSwiftUI or another hypothetical client where the actual bits are assigned meaning.
In OG, these are only used by OGSubgraphIsDirty, OGSubgraphIntersects, OGSubgraphApply and OGSubgraphUpdate with the effect that those functions only consider attributes whose flags match the passed in mask (or all attributes if the passed in mask is 0x00).
As for the name, the JSON returned from OGGraphDescription includes these under the key "subgraph_flags".
| OGAttributeTypeFlagsExternal = 1 << 4, | ||
| OGAttributeTypeFlagsAsyncThread = 1 << 5, | ||
| }; | ||
| } OG_SWIFT_NAME(Flags); |
There was a problem hiding this comment.
The name should not be Flags.
There was a problem hiding this comment.
I am basing it off this section:
struct __C._AttributeType {
var self_id: __C.Metadata
var value_id: __C.Metadata
var update: __C._AGClosureStorage
var vtable: Swift.UnsafePointer<__C._AttributeVTable>
var flags: __C.Flags
var internal_offset: Swift.UInt32
var value_layout: Swift.Optional<Swift.UnsafePointer<Swift.UInt8>>
}
There was a problem hiding this comment.
If a type is XXYYZZ marks as NS_SWIFT_NAME(YY.ZZ), the name you will see is __C.ZZ.
So I suggest changing the name to OGAttributeType.Flags
There was a problem hiding this comment.
Ahh I didn't know that. Sure I'll change it to OGAttributeType.Flags!
| OGCounterQueryTypeCreatedNodes, | ||
| OGCounterQueryTypeSubgraphs, | ||
| OGCounterQueryTypeCreatedSubgraphs, | ||
| } OG_SWIFT_NAME(OGGraphRef.CounterQueryType); |
There was a problem hiding this comment.
Also change the enum to OGGraphCounterQueryType
| OGTraceEventsNamed = 2, | ||
| OGTraceEventsDeadline = 3, | ||
| OGTraceEventsCompareFailed = 4, | ||
| }; |
| void (*_Nullable passed_deadline)(void *_Nullable context); | ||
|
|
||
| void (*_Nullable compare_failed)(void *_Nullable context, OGAttribute attribute, OGComparisonState comparisonState); | ||
| } OGTrace; |
| OGValueStateMainRef = 1 << 5, | ||
| OGValueStateRequiresMainThread = 1 << 6, | ||
| OGValueStateSelfModified = 1 << 7, | ||
| }; |
There was a problem hiding this comment.
The same as I commented above. Why the cases is removed and use rawValue here?
There was a problem hiding this comment.
I agree the tests are much more verbose now. But I disagree it is a concern of OG to define what each flag mean for the same reasons above. What if we define an extension in the test target so the test at least is more readable?
There was a problem hiding this comment.
Got it. I'll do it in the upstream then.
What if we define an extension in the test target so the test at least is more readable?
Yeah. Go ahead.
|
When we are ready with the final comment, here is the merge process @jcmosc :
|
# Conflicts: # Sources/OpenGraph_SPI/Graph/GraphDescription.mm # Sources/OpenGraph_SPI/include/OGGraphDescription.h # Conflicts: # Sources/OpenGraphCxx/include/OpenGraph/OGAttributeType.h # Sources/OpenGraphCxx/include/OpenGraph/OGClosure.h # Sources/OpenGraphCxx/include/OpenGraph/OGValue.h
Kyle-Ye
force-pushed
the
update-public-interface
branch
from
37a0caa to
a46e09f
Compare
2 participants
No description provided.