Skip to content

Keep Objective-C configuration copies independent and owned - #1539

Closed
bmehta001 wants to merge 2 commits into
microsoft:mainfrom
bmehta001:bhamehta/fix-objc-config-copy
Closed

bmehta001 wants to merge 2 commits into
microsoft:mainfrom
bmehta001:bhamehta/fix-objc-config-copy

Conversation

@bmehta001

@bmehta001 bmehta001 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Give each ODWLogConfiguration copy its own wrapped configuration instead of sharing the file-scope pointer.
  • Own the copied C++ configuration for the lifetime of its Objective-C wrapper, avoiding the existing leak.
  • Add an XCTest regression that creates multiple copies and checks independent values and host settings.

Supersedes the fix proposed in #1292 with ownership and regression coverage.

Validation

  • git diff --check passed.
  • iOS XCTest requires Apple tooling; the iOS CI matrix will build and run the suite.

Move the wrapped configuration from file scope to a per-instance owning pointer, preventing later copies from redirecting earlier wrappers and leaking prior allocations.

Files: wrappers/obj-c/ODWLogConfiguration.mm, wrappers/obj-c/ODWLogConfiguration_private.h, tests/unittests/obj-c/ODWLogConfigurationTests.mm

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: a668528d-5dc9-4648-a753-95c0235cc103
Check presence through HasConfig rather than converting an unset Variant to a string; this preserves the regression assertion without crashing debug iOS tests.

Files: tests/unittests/obj-c/ODWLogConfigurationTests.mm

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: a668528d-5dc9-4648-a753-95c0235cc103
@bmehta001

Copy link
Copy Markdown
Contributor Author

All checks passed, including the iOS XCTest matrix. As requested, the validated fix and regression test have been merged into the branch for #1529 (commit 818f376); closing this PR to avoid maintaining a duplicate change.

@bmehta001 bmehta001 closed this Sep 24, 2026
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.

1 participant