Refactor/property object - #6
Draft
JakaMohorko wants to merge 34 commits into
Draft
Conversation
Move isChildProperty, splitOnFirstDot, splitOnLastDot, parseIndex, getPropertyNameInfo, getPropNameWithoutIndex and the PropertyNameInfo struct into daq::details in the new property_object_helpers.h header. These are stateless functions with no dependency on the template parameters, so they no longer need to live inside the class. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move the type-check group (checkContainerType, checkStructType, checkEnumerationType, checkSelectionValues, checkPropertyTypeAndConvert), the coercion/validation group (coercePropertyWrite, validatePropertyWrite, coerceMinMax) and the reference-resolution group (checkForRefPropAndGetBoundProp, hasDuplicateReferences, checkIsReferenced) into daq::details in property_object_helpers.h. All are stateless functions of their arguments with no dependency on the class template parameters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Child property access (a.b.c.prop) previously recursed through the full operation at every path level - each hop re-entered the internal set/get/clear, re-parsed the remaining path, and re-acquired locks. getParentObject now walks the dot-separated path iteratively: it splits off the leaf name, resolves one segment per hop (first hop through the object's own internal getter, further hops through the child's public getPropertyValue), and returns the direct parent of the leaf property. The operation-specific machinery then runs exactly once, at the leaf. All nine dispatch sites (set, protected set, clear, get, selection set, getProperty, both event getters) now share the resolver and keep only their operation-specific leaf action. getChildPropertyValue is removed. hasProperty and selection get inherit the iterative walk through getPropertyValue/getProperty. Per-hop semantics (ref-property resolution, read events, permission checks, error codes) are unchanged. Intermediate objects' locks are no longer held across the leaf operation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
setPropertySelectionValueInternal and getPropertySelectionValueInternal carried ~135 lines of selection-type branching. The conversions now live in details::selectionValueToKey (user value -> stored index/key) and details::selectionKeyToValue (stored key -> selection value), next to checkSelectionValues. Both internals reduce to: resolve the bound property, convert, and delegate to the normal value get/set path. Error codes are unchanged; helper messages now name the resolved property instead of the raw input path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 90-line method mixed name parsing, ref resolution, effective-name computation, updating-stack reads, default-value fallback with list indexing, container cloning, and read-event dispatch. Three of those concerns move to pure helpers in property_object_helpers.h: - details::buildEffectivePropertyName - the storage/lookup name for a resolved property, preserving [index] suffixes (resolves the long-standing TODO to extract this) - details::readDefaultPropertyValue - default-value fallback, including indexing into list defaults - details::cloneContainerValue - defensive clone of list/dict values on read What remains is a linear orchestrator: parse -> bind -> read (updating stack or local value) -> default fallback -> clone -> read event. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The function is a no-op for non-container values; the name now says so. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getPropertyAndValueInternal conflated three concerns behind two bool flags: binding the property (name parse, lookup, reference resolution), reading its value, and firing read events. Split it into: - getBoundPropertyInternal: lookup + reference resolution, outputs the bound property, effective storage name, and bracket suffix - readPropertyValueInternal: updating-stack/local/default value read, never fires events Callers now invoke callPropertyValueRead explicitly where read events are wanted, making endApplyUpdate's read-event firing during update application visible (kept for behavior parity, marked TODO). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shouldWriteLocalValue and writeLocalValue each hand-rolled the same compare-against-property-default check; extract it into differsFromDefaultValue and flatten writeLocalValue branching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rtyValues All read-path callers were calling getBoundPropertyInternal and readPropertyValueInternal back to back; add a name-based readPropertyValueInternal overload that does both so call sites lose the effectiveName/bracket boilerplate. clearPropertyValuesInternal already holds a bound, non-reference property from the getAllProperties loop, so re-looking it up and re-resolving it was pure waste (its property out-param was never used); it now reads the value directly via the bound-property overload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The member holds the begin/endUpdate batch queue, but its old name read as if it belonged to updatePropertyStack (the write-event reentrancy guard), which is an unrelated mechanism. Also updates the references in the commented-out config client block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
callBeginUpdateOnChildren and callEndUpdateOnChildren duplicated the same skip-non-objects, skip-frozen loop; extract it into forEachUnfrozenChildObject. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both were ~35 identical lines differing only in the event map, the child-dispatch target, and the error string; fold them into getPropertyValueEventInternal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hasProperty was the last site hand-rolling parent resolution (splitOnLastDot plus a full getPropertyValue on the parent path). NOTFOUND and NOINTERFACE now uniformly map to hasProperty = False; previously a non-object segment in the middle of a deep path errored while one at the end reported False. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Delete the throwing getUnboundProperty variant; all sites now use the null-returning lookup with an explicit NOTFOUND error. This makes the set path consistent with the clear path (its post-resolution not-found check was previously unreachable for missing properties, since the throwing lookup fired first with a different message) and normalizes the error text to a single wording. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pre-check existed to guard the old throwing lookup; since the convergence on getUnboundPropertyOrNull it duplicated the same localProperties/objectClass lookup with the identical error result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ject branch The nested-object clear loop existed twice, differing only in which clear call it made per child property. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rite Both fired the same class-level, per-property, and any-property event sequence; fold it into firePropertyValueEvents. Guarding is unchanged: only the per-property write tier runs under daqTry, class-level and any-property handlers still propagate exceptions (the resulting update-stack leak on throw is a known issue to fix separately). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…meters The 9-argument overload existed only because CloneParameters was a nested type, so each template instantiation had its own and clone() could not pass one across instantiations. Hoist the struct (and the event emitter aliases it needs) to namespace scope as PropertyObjectCloneParameters, keep the in-class CloneParameters alias, and fold the 9-argument body into the single overload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The custom-order branch duplicated the append-the-remaining-properties loop; with an empty customOrder the first loop is a no-op, so the branch is unnecessary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The name is the queried name rewritten onto the resolved property after reference resolution; resolved states the transformation, effective did not. Also renames buildEffectivePropertyName accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
It is an implementation detail of the base begin/endUpdate propagation; subclass overrides extend to components, not propValues children, and have no use for it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Since child object properties are materialized into propValues at add time (with non-core defaults replaced by inert clones and core-property defaults aliasing the propValues entry), the defaults iterated by the second loop either never had their trigger enabled or were already disabled by the propValues loop. Leftover from the pre-rework value-on-demand semantics; enableCoreEventTrigger never had a counterpart walk. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A handler throwing from the class-level or any-property write tier unwound past unregisterPropertyUpdating, leaking the update-stack entry. Since all public reads consult the updating stack first, the leaked entry became a phantom value: every read returned the never-committed value while propValues (the state that serializes and clones) stayed empty, and re-setting that value was silently ignored. All write tiers now run under daqTry (all tiers still fire, first error wins) and callPropertyValueWrite unregisters on unwind as a backstop. Pinned by a test verified to fail against the previous implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
clearPropertyValue(child.Prop) during begin/endUpdate was queued on the initiating object and applied only after the child had already ended its own update: the clear ran with isUpdating=false, outside the child's update scope, and the child's endUpdate event never reported it. Sets already dispatch child paths immediately so the operation lands in the updating child's own batch (behavior pinned by ConfigProtocolIntegrationTest.BeginEndUpdateNestedPropertyObjectOrder); clears now do the same. Also clears the ignored error info when endApplyUpdate cannot re-read an applied value on this object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
clearPropertyValue on an object-type property hand-rolled a per-child loop that cleared every child unconditionally: with a read-only child present the non-protected clear threw ACCESSDENIED, and reference properties were not skipped - while clearPropertyValues on the same nested object politely skips both. Delegate to the nested object''s clearPropertyValues/clearProtectedPropertyValues so there is one clear semantics. Pinned by a test verified to fail against the previous implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The lookup + reference-resolution + not-found-error + name-rebind sequence appeared verbatim in setPropertyValueInternal, clearPropertyValueInternal, and setPropertySelectionValueInternal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The five type/compatibility checks, the three coercion/validation steps, and the defensive container clone move to the helpers header as one function, leaving only the object-configuration arm (which needs member state) in setPropertyValueInternal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Behavior-neutral batch: delete the dead splitOnFirstDot (unused since the getParentObject rewrite), express getPropertyNameInfo via getPropNameWithoutIndex instead of a second bracket scan, default PropertyNameInfo::index to the -1 sentinel, flatten checkSelectionValues and coerce/validatePropertyWrite with guard clauses, add a null guard to cloneIfContainerValue, drop unnecessary .template disambiguators, make checkForRefPropAndGetBoundProp take the property by const reference, avoid StringPtr copies in reference loops, unify bool return types, fix the misleading dict error text in selectionKeyToValue, and order the includes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract the post-bind commit tail of setPropertyValueInternal into writeBoundPropertyValue and call it directly from the selection-set path with the property it already bound for key conversion, instead of re-entering setPropertyValueInternal and binding a second time. Frozen-check and batch-queue ordering are preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Split the 92-line function into collectAllProperties (class + local property gathering), bindAndFilterProperties (owner binding and visibility/reference filtering), and applyCustomPropertyOrder (customOrder-first ordering). getPropertiesInternal is now a four-line orchestrator. No behavior change; findProperties already delegated here and needed no edits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PropertyObjectClassImpl::hasDuplicateReferences was dead code - declared and defined but never called; removed. The builder now delegates to a shared details::hasDuplicateReferences overload in property_object_helpers.h that takes the property list to scan, replacing its member copy of the same set-intersection algorithm. The existing object-level overload (getIsReferenced-based) is a different check and stays as is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Define the legacy generation in terms of its counterparts: getRecursiveConfigLock now wraps getRecursiveConfigLock2 instead of repeating the getRecursiveLockGuard/checkErrorInfo sequence, and the std-mutex accessors are single-expression. A comment marks the non-2 set as the legacy generation pending its cross-repo retirement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Applied to property_object_impl.h and property_object_helpers.h: - Single-statement if/else bodies have no braces, regardless of content - plain calls, returns of openDAQ error macros (return DAQ_MAKE_ERROR_INFO(...)), DAQ_THROW_EXCEPTION, and OPENDAQ_RETURN_IF_FAILED alike (all macros involved are hygienic: expression, single throw, or do-while(0)). - Statements wrapped across rows for length gain braces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JakaMohorko
force-pushed
the
refactor/property_object
branch
from
August 11, 2026 06:57
2d257d8 to
584228a
Compare
bindForWrite -> bindProperty: the old suffix only encoded the absence of bracket parsing, which the name could not convey; a contract comment now covers that difference from getBoundPropertyInternal. writeBoundPropertyValue -> checkAndSetPropertyValue: the function is the full commit stage of a set (access check, coercion/validation, object clone configuration, event-firing commit or silent write), which plain write undersold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.