[Clippy] Modernise random/guid usage, fix discarded Guid bug, and remove dead code#244
Conversation
- Replace `new Random()` with `Random.Shared` in GenStyleIdFromStyleName for
thread safety and to avoid unnecessary allocations (DocumentBuilder.cs)
- Fix discarded Guid.NewGuid() assignments in WmlComparer text-box ancestor
processing — result was computed but never assigned, leaving `thisUnid` null
(WmlComparer.Private.Methods.ProduceDocument.cs)
- Simplify `Guid.NewGuid().ToString().Replace("-", "")` to
`Guid.NewGuid().ToString("N")` for clarity and efficiency (WmlComparer.Private.Methods.Util.cs)
- Remove `#if false` dead code in FieldRetriever.cs — the old non-caching
implementation has been superseded by the cached version
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Modernizes random/GUID generation patterns in Clippit’s Word-processing utilities and comparer pipeline, fixes a latent bug where generated GUIDs were discarded, and removes long-dead conditional code.
Changes:
- Use
Random.SharedinDocumentBuilderto avoid per-callRandomallocations and weak seeding behavior. - Replace
Guid.NewGuid().ToString().Replace("-", "")withGuid.NewGuid().ToString("N")for lower-allocation GUID formatting. - Fix two lambdas in comparer document production where a newly generated GUID was previously discarded, allowing null ancestor IDs to leak.
- Remove
#if falsedead code inFieldRetriever.InstrText.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Clippit/Word/DocumentBuilder.cs | Switch style-id suffix generation to Random.Shared. |
| Clippit/FieldRetriever.cs | Remove dead #if false non-caching implementation block. |
| Clippit/Comparer/WmlComparer.Private.Methods.Util.cs | Use Guid.ToString("N") for part URI name generation. |
| Clippit/Comparer/WmlComparer.Private.Methods.ProduceDocument.cs | Fix discarded GUID result when ancestor Unid is missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot fix inline comments |
…torUnids Agent-Logs-Url: https://github.com/sergey-tihon/Clippit/sessions/e8cd4284-e954-4e6f-84c0-8b361d9110c5 Co-authored-by: sergey-tihon <1197905+sergey-tihon@users.noreply.github.com>
Fixed in commit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🤖 This is an automated PR from Clippy, an AI assistant for this repository.
Summary
Four focused code-quality improvements across the library:
1.
Random.Sharedinstead ofnew Random()(DocumentBuilder.cs)GenStyleIdFromStyleNamewas calling(new Random()).Next(990). This allocates a newRandominstance on every call and can produce identical sequences when called in rapid succession on older runtimes. UsingRandom.Sharedis the idiomatic .NET 6+ approach: no allocation, thread-safe, and backed by a properly seeded shared instance.2. Fix discarded
Guid.NewGuid()result — latent bug (WmlComparer.Private.Methods.ProduceDocument.cs)Two
Selectlambdas in the text-box ancestor-unid processing contained:The assignment to
thisUnidwas accidentally dropped, sonullwould be returned and inserted into theAncestorUnidsarray — potentially causing downstreamNullReferenceExceptions or incorrect comparison results. Fixed tothisUnid = Guid.NewGuid().ToString("N");.3. Simplify
Guid.ToString("N")(WmlComparer.Private.Methods.Util.cs)Guid.NewGuid().ToString().Replace("-", "")→Guid.NewGuid().ToString("N"). The"N"format specifier produces the same 32-character lowercase hex string without hyphens, but avoids the extra string allocation fromReplace.4. Remove
#if falsedead code (FieldRetriever.cs)The original non-caching implementation of
InstrTextwas left behind a#if falseblock with a comment noting the caching version is "significantly faster". This block has been dead code for a long time and can be safely removed.Test Status