Skip to content

Fix false-positive ArrayPtr dispose warning#370

Open
johnml1135 wants to merge 2 commits intosillsdev:masterfrom
johnml1135:fix/arrayptr-false-positive-dispose-warning
Open

Fix false-positive ArrayPtr dispose warning#370
johnml1135 wants to merge 2 commits intosillsdev:masterfrom
johnml1135:fix/arrayptr-false-positive-dispose-warning

Conversation

@johnml1135
Copy link
Contributor

@johnml1135 johnml1135 commented Mar 12, 2026

Summary

Suppress the ArrayPtr finalizer warning for non-owning wrappers such as ArrayPtr.Null while preserving the warning for managed-owned unmanaged buffers.

Why

FieldWorks debug sessions were reporting a misleading "Missing Dispose() call for ArrayPtr" message at shutdown even when the finalized ArrayPtr instance did not own unmanaged memory.

Validation

  • Rebuilt the local liblcm net462 runtime overlay
  • Verified the updated SIL.LCModel DLL and PDB files were copied into FieldWorks Output/Debug
  • Confirmed the change is limited to ArrayPtr warning behavior

This change is Reviewable

Copilot AI review requested due to automatic review settings March 12, 2026 20:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refines ArrayPtr’s finalizer-time debug warning so shutdown logs don’t report “Missing Dispose()” for wrapper instances that do not own unmanaged memory (e.g., ArrayPtr.Null), while keeping the warning for instances responsible for freeing unmanaged buffers.

Changes:

  • Remove legacy commented-out “hack” related to excluding the null instance from finalizer behavior.
  • Gate the “Missing Dispose()” Debug.WriteLineIf warning on m_ownMemory to avoid warning for non-owning wrappers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// If you're getting this line it means that you forgot to call Dispose().
Debug.WriteLineIf(!disposing,// && this != s_Null, // TODO (Hasso) 2016.11: is this the only place this ugly hack is needed?
// Only warn for finalized instances that actually own allocated memory.
Debug.WriteLineIf(!disposing && m_ownMemory,
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The new warning condition uses m_ownMemory, but m_ownMemory == true does not necessarily mean unmanaged memory was actually allocated (e.g., new ArrayPtr() sets m_ownMemory = true while m_ptr is still IntPtr.Zero until Resize is called). As written, finalized instances created via the default ctor but never allocated could still emit the warning, which contradicts the new comment and can still produce false positives. Consider including an allocation check (e.g., m_ptr != IntPtr.Zero / m_Size >= 0) or adjust the comment to match the actual condition.

Suggested change
Debug.WriteLineIf(!disposing && m_ownMemory,
Debug.WriteLineIf(!disposing && m_ownMemory && m_ptr != IntPtr.Zero,

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

:lgtm:

@jasonleenaylor reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on johnml1135).

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.

3 participants