Conversation
Change-Id: Id4eabbcd70df8b594d26ee2fba777783f518f070
Change-Id: I89db58bf48ffb14b8c8edcaed8ae26549a7a522a
johnml1135
left a comment
There was a problem hiding this comment.
Claude - Sonnet 5 — automated code review.
Two additional findings sit on lines outside this PR's diff hunks (pre-existing code untouched here), so they can't be attached as inline comments — noting them here instead:
- Reuse (
Src/Utilities/pcpatrflex/ToneParsFLExDll/ToneParsInvoker.cs~line 101):ConvertNameToUseAnsiCharactersis a byte-for-byte duplicate ofXAmpleParser.ConvertNameToUseAnsiCharacters, explicitly commented as "borrowed from SIL.FieldWorks.WordWorks.Parser (ParserCore.dll)". Since this PR already dedupesProcessParseResultsagainst the same class, andToneParsInvokernow holds a liveXAmpleParserreference, this is a natural candidate to dedupe too (note:XAmpleParser's version isinternal static, so calling through would need an accessibility change). - Cleanup (
Src/Utilities/pcpatrflex/ToneParsFLExDll/ToneParsInvoker.csline 22):using System.Xml.Linq;is now dead — it was the only consumer's (XElement/Descendants) namespace, and that code was deleted by this PR. No remaining use of anySystem.Xml.Linqtype in the file.
| XAmpleParser = xAmpleParser; | ||
| if ( XAmpleParser == null ) | ||
| { | ||
| XAmpleParser = new XAmpleParser(Cache, null); |
There was a problem hiding this comment.
Claude - Sonnet 5: This fallback constructs a full XAmpleParser (native XAmple engine init + a ParserModelChangeListener registered on Cache.DomainDataByFlid), but ToneParsInvoker has no IDisposable/Dispose and never disposes it — a genuine resource leak. Worse, it's not an edge case: in ToneParsFLExForm.cs, m_XAmpleParser is only populated on the HermitCrab code path, so whenever XAmple is the active parser, every "Parse Text"/"Parse Segment"/"Show Log" click passes null here and leaks a new native handle + listener each time.
Separately: this fallback passes dataDir = null, and XAmpleParser.Update() does Path.Combine(m_dataDir, "Configuration", "Grammar") with no null guard. Nothing in ToneParsInvoker calls Update()/Reset() today so this is latent, but every other production caller of new XAmpleParser(...) passes a real dataDir — this is a new, unguarded landmine if that ever changes.
| return result; | ||
| } | ||
|
|
||
| public ParseResult ProcessParseResults(ref StringBuilder results) |
There was a problem hiding this comment.
Claude - Sonnet 5: The ref on StringBuilder results is functionally unnecessary — StringBuilder.Replace mutates in place and returns the same instance, and neither call site (ParseWord here, or ToneParsInvoker.SaveResultsInDatabase) reads results after the call. A plain string results parameter would be simpler and would avoid the caller in ToneParsInvoker.cs (SaveResultsInDatabase) having to allocate a throwaway StringBuilder purely to satisfy this signature.
Also: this method doesn't call CheckDisposed(), unlike Update()/Reset()/ParseWord()/ParseWordXml() on this same DisposableBase-derived class. Lower severity — this mirrors a pre-existing gap in the code this PR deletes (which had the same CheckDisposed() omitted, with a //TODO: fix! comment), and there's no demonstrated live path where a disposed instance is called through today — but now that this method is called from another assembly (ToneParsInvoker), it's worth closing for consistency with the rest of the public API.
Now that the TonePars utility is part of FLEx, we can have it use the same code that XAmple does when processing its result.
This change is