src/NLog/MessageTemplates/TemplateEnumerator.cs (2)

65-81: Constructor pre-scan is correct — minor nit on _length reuse.

Traced the pre-scan against the callers in Logger.cs, LogMessageTemplateFormatter.cs, and MessageTemplateParameters.cs, as well as the negative test cases in ParserTests.ThrowsTemplateParserException — all exception paths still fire via MoveNext, and the literal-only and empty-template cases degrade gracefully.

Small consistency nit: the else branch reassigns _template.Length twice when _length is already cached one line above.

♻️ Optional consistency tweak

             else
             {
-                _position = _template.Length;
-                _literalLength = _template.Length;
+                _position = _length;
+                _literalLength = _length;
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/MessageTemplates/TemplateEnumerator.cs` around lines 65 - 81, The
constructor in TemplateEnumerator caches _length but the else branch redundantly
calls _template.Length again; update the else branch to use the cached _length
when assigning _position and _literalLength to avoid inconsistent access
patterns—edit the TemplateEnumerator(string template) constructor to set
_position = _length and _literalLength = _length (referencing the _length field,
not _template.Length) while leaving the existing first-branch logic and other
fields (_template, _position, _literalLength, _current) unchanged.

---

374-411: Signed/unsigned split looks good; note silent overflow beyond 10 digits.

The contract (caller guarantees c is '0'..'9' before invoking ReadUnsignedInt) is honored by both call sites (ReadSignedInt and the positional branch of ParseName). The 12-iteration cap yields up to 13 consumed digits before throwing — values in the 11–13 digit range silently wrap (int overflow is unchecked). This matches the prior ReadInt behavior in practice, so it is not blocking, but a checked block (or tightening the cap to 10) would produce a clearer TemplateParserException for pathological inputs instead of a garbage index/alignment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/MessageTemplates/TemplateEnumerator.cs` around lines 374 - 411,
ReadUnsignedInt can silently overflow for 11–13 digit inputs; update it so it
fails with a TemplateParserException instead of producing a wrapped/garbage int.
Either (a) enforce a max of 10 digits (first digit is handled before the loop —
e.g. loop at most 9 more digits) and throw TemplateParserException when more
digits are present, or (b) perform the multiplication/add in a checked context
and catch OverflowException to rethrow as TemplateParserException. Apply this
change in ReadUnsignedInt (and ensure ReadSignedInt behavior remains consistent)
so pathological numeric token lengths are rejected deterministically.

src/NLog/MessageTemplates/TemplateEnumerator.cs (2)

192-206: Losing the Skip('}') invariant check.

Replacing Skip('}') with a bare _position++ drops the Debug.Assert(_template[_position] == '}') that guarded the parser invariant "after ParseName/format parsing we are sitting on }". Given every path leading here (format parser unread, direct }/: peek, alignment+}) is supposed to land exactly on }, keeping the assert has zero release-build cost and would catch regressions in future refactors.

♻️ Retain the debug-only invariant check

-            _position++; // Consume closing '}'
+            Skip('}'); // Consume closing '}' (Debug.Assert validates the invariant)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/MessageTemplates/TemplateEnumerator.cs` around lines 192 - 206, The
replacement of Skip('}') with a bare _position++ in ParseHole removed the
debug-only invariant that ensures the parser is positioned on '}' after
ParseName/format parsing; restore the guard by asserting the current character
is '}' before advancing (e.g., reintroduce Debug.Assert(_template[_position] ==
'}') or call the existing Skip('}') helper) so ParseHole (and its usage of
_position++/Skip) re-establishes the invariant and then increments the position.

---

103-108: Reset() no longer mirrors the constructor's fast-forward.

After this change, Reset() leaves _position = 0, _literalLength = 0, so the first MoveNext() after a reset will re-scan via ParseTextPart instead of jumping straight to the first delimiter. This is functionally correct but gives up the optimization you just added for the reset path. Consider either reusing the same pre-scan, or documenting that Reset() is a slow-path reset.

♻️ Optional alignment with constructor

 public void Reset()
 {
-    _position = 0;
-    _literalLength = 0;
     _current = default(LiteralHole);
+    var firstDelimiter = _template.IndexOfAny(TextDelimiters);
+    _position = firstDelimiter >= 0 ? firstDelimiter : _length;
+    _literalLength = _position;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/MessageTemplates/TemplateEnumerator.cs` around lines 103 - 108,
Reset() currently sets _position=0 and _literalLength=0 which forces the next
MoveNext() to re-scan via ParseTextPart and loses the constructor's fast-forward
optimization; update Reset() in TemplateEnumerator to re-run the same initial
pre-scan the constructor performs (or call the same initialization helper) so
_position/_literalLength/_current are advanced to the first delimiter like the
constructor does, or explicitly document that Reset() is a slow-path reset;
reference Reset(), the constructor, MoveNext(), and ParseTextPart when making
the change.

src/NLog/MessageTemplates/TemplateEnumerator.cs (1)

103-108: Optional: mirror the constructor's fast-forward in Reset().

After Reset() the enumerator is left at _position = 0, _literalLength = 0 rather than fast-forwarded to the first delimiter, so the first MoveNext after a Reset takes the slower ParseTextPart path. Functionally correct (behavior is identical), but it leaves the two entry points semantically out of sync. If you want to keep them aligned, factor the init into a shared helper.

♻️ Possible alignment

         public void Reset()
         {
-            _position = 0;
-            _literalLength = 0;
             _current = default(LiteralHole);
+            var firstDelimiter = _template.IndexOfAny(TextDelimiters);
+            if (firstDelimiter >= 0)
+            {
+                _position = firstDelimiter;
+                _literalLength = firstDelimiter;
+            }
+            else
+            {
+                _position = _length;
+                _literalLength = _length;
+            }
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/MessageTemplates/TemplateEnumerator.cs` around lines 103 - 108,
Reset() currently just zeroes _position/_literalLength/_current leaving the
enumerator before the first delimiter so the first MoveNext() hits
ParseTextPart; update Reset() to mirror the constructor’s fast-forward logic by
extracting the initial fast-forward into a shared helper (e.g., Initialize or
FastForwardToDelimiter) and call it from both the constructor and Reset(),
ensuring Reset(), the constructor, and MoveNext()/ParseTextPart are semantically
aligned while preserving existing fields (_position, _literalLength, _current,
LiteralHole).