Skip to content

Switch ItemExpressionCapture to a struct #12150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Erarndt
Copy link
Contributor

@Erarndt Erarndt commented Jul 10, 2025

Fixes #

Context

We can avoid several allocations by changing the usage of ItemExpressionCapture. Switching from a class to a struct avoids a large number of allocations, and we can further reduce the allocations using a struct-based enumerator rather than constructing a list.

Before:
image

After:
image

Changes Made

Testing

Notes

@Copilot Copilot AI review requested due to automatic review settings July 10, 2025 22:23
Copy link
Contributor

@Copilot 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 changes ItemExpressionCapture from a class to a struct to reduce heap allocations, replaces the list-based item-expression capture with a struct-based enumerator, and updates all callers and tests to use the new pattern.

  • Convert ItemExpressionCapture to a struct and make capture methods return nullable structs
  • Introduce ReferencedItemExpressionsEnumerator in place of List<ItemExpressionCapture>
  • Update callers and unit tests to use MoveNext() and Current

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Build/Evaluation/LazyItemEvaluator.cs Unwrap nullable struct before passing to helper
src/Build/Evaluation/ItemSpec.cs Unwrap nullable struct when constructing fragments
src/Build/Evaluation/ExpressionShredder.cs Define ReferencedItemExpressionsEnumerator and refactor enumeration code
src/Build/Evaluation/Expander.cs Adapt callers to use the new struct enumerator
src/Build.UnitTests/Evaluation/ExpressionShredder_Tests.cs Update tests to use MoveNext() and Current
Comments suppressed due to low confidence (3)

src/Build/Evaluation/ExpressionShredder.cs:103

  • [nitpick] Consider adding XML doc-comments for MoveNext and Current to clarify usage, and indicate that reset behavior is not supported.
        internal struct ReferencedItemExpressionsEnumerator

src/Build/Evaluation/ExpressionShredder.cs:107

  • [nitpick] The field currentIndex is a bit generic; consider renaming to something like scanPosition or cursor to better convey its role in the enumerator.
            private int currentIndex;

src/Build/Evaluation/ExpressionShredder.cs:103

  • [nitpick] Rather than a standalone struct, consider implementing IEnumerator<ItemExpressionCapture> to align with standard .NET enumeration patterns and enable use in foreach.
        internal struct ReferencedItemExpressionsEnumerator

@@ -571,11 +571,12 @@ private static void VerifyAgainstCanonicalResults(string test, IDictionary actua
public void ExtractItemVectorTransform1()
{
string expression = "@(i->'%(Meta0)'->'%(Filename)'->Substring($(Val)))";
List<ExpressionShredder.ItemExpressionCapture> expressions = ExpressionShredder.GetReferencedItemExpressions(expression);
ExpressionShredder.ReferencedItemExpressionsEnumerator expressions = ExpressionShredder.GetReferencedItemExpressions(expression);
expressions.MoveNext();
Copy link
Preview

Copilot AI Jul 10, 2025

Choose a reason for hiding this comment

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

Before accessing expressions.Current, add an assertion such as Assert.True(expressions.MoveNext()); to ensure the enumerator actually advanced.

Suggested change
expressions.MoveNext();
Assert.True(expressions.MoveNext());

Copilot uses AI. Check for mistakes.

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