Skip to content
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

fix-text-differ #8215

Merged
merged 1 commit into from
Feb 4, 2023
Merged

fix-text-differ #8215

merged 1 commit into from
Feb 4, 2023

Conversation

LunicLynx
Copy link
Contributor

Encountered an IndexOutOfRange Exception here, when formatting a large .razor file.

The debugger showed that the pages array had 3 entries, the expression index % PageSize evaluated to some high values like 23300 % 20480.

I think this should be an integer division instead of a modulo operation.

@LunicLynx LunicLynx requested a review from a team as a code owner February 3, 2023 07:55
@@ -118,7 +118,7 @@ private static void ReturnArray(int[] array, bool clearArray = false)
// Does this index fall within page? If not, acquire the appropriate page.
if (index < page.Start || index >= page.Start + page.Length)
{
page = _pages[index % PageSize];
page = _pages[index / PageSize];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic only makes sense with division. Also this error does not happen with small files because of the if.

@davidwengier
Copy link
Contributor

Too early for me to think about math. Paging @DustinCampbell

Is this issue in P1? We need to service it if so.

Also, perhaps a test? Even if it just created a really big file, formats it, and at least we'll know there aren't any exceptions thrown?

@DustinCampbell
Copy link
Member

Big "oops" for me! Although, I would've expected to see this show up in the benchmarks, which test a ~315KB file.

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

I'll submit a follow-up PR with a test. Let's not block this fix for that.

@DustinCampbell DustinCampbell merged commit 6b65fb8 into dotnet:main Feb 4, 2023
DustinCampbell added a commit to DustinCampbell/razor that referenced this pull request Feb 4, 2023
DustinCampbell added a commit to DustinCampbell/razor that referenced this pull request Feb 4, 2023
DustinCampbell added a commit to DustinCampbell/razor that referenced this pull request Feb 6, 2023
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.

4 participants