Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix redundant iteration while splitting lines#14720

Closed
hez2010 wants to merge 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:fix-generategineswithoutwordwrapiSazonov/PowerShell:fix-generategineswithoutwordwrapCopy head branch name to clipboard
Closed

Fix redundant iteration while splitting lines#14720
hez2010 wants to merge 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:fix-generategineswithoutwordwrapiSazonov/PowerShell:fix-generategineswithoutwordwrapCopy head branch name to clipboard

Conversation

@hez2010

@hez2010 hez2010 commented Feb 6, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

@@ -87,8 +87,15 @@ internal DisplayCellsPSHost(PSHostRawUserInterface rawUserInterface)

internal override int Length(string str, int offset)
{

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

hez2010 commented Feb 14, 2021

Copy link
Copy Markdown
Contributor Author

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

@iSazonov

Copy link
Copy Markdown
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

hez2010 commented Feb 16, 2021

Copy link
Copy Markdown
Contributor Author

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.