Skip to content

Conversation

@hez2010
Copy link
Contributor

@hez2010 hez2010 commented Feb 6, 2021

PR Summary

Fix redundant iteration while splitting lines to prevent assert failure.

PR Context

We split a line to fit in a windows width and if on last iteration we did get zero length tail we did next iteration but we should stop.

Borrowed from @iSazonov, see #13551 (comment)

Fixes #13551

Todo:

  • [ ] Add tests: can only repro in a specified range of width of window while rendering table, I don't know how to test it automatically without user interaction.

PR Checklist

We split a line to fit in a windows width and if on last iteration we did get zero length tail we did next iteration but we should stop.
@ghost ghost assigned TravisEz13 Feb 6, 2021
@hez2010 hez2010 marked this pull request as ready for review February 10, 2021 16:38
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 10, 2021
for (int k = 0; k < lines.Length; k++)
{
if (lines[k] == null || displayCells.Length(lines[k]) <= firstLineLen)
var currentLine = lines[k];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var currentLine = lines[k];
string currentLine = lines[k];

We should respect EditorConfig setting: csharp_style_var_for_built_in_types = false.

}

internal override int Length(string str, int offset)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes to this method necessary to fix the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. But it's a refactor to Length to reduce ops while str is empty.


if (offset < 0 || offset >= str.Length)
{
throw PSTraceSource.NewArgumentException(nameof(offset));
Copy link
Contributor

Choose a reason for hiding this comment

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

In particular this exception.

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 14, 2021

@iSazonov can I push to your branch? or I have to create another PR from my fork and close this one.

@iSazonov
Copy link
Collaborator

@iSazonov can I push to your branch? or I have to create another PR from my fork and close this one.

It is better to use your fork if you want to add something. Please avoid in-depth refactoring.

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 16, 2021

I'm closing this PR and create another one from my folk later.

@hez2010 hez2010 closed this Feb 16, 2021
@iSazonov iSazonov deleted the fix-generategineswithoutwordwrap branch October 24, 2021 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log WG-Engine-Format

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: PowerShell cannot handle a file with long non-ascii file name

4 participants