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

Make Fix-All use normal text-diffing as the TreeDiffer produces some very bad results. #17497

Merged
merged 11 commits into from
Mar 7, 2017

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi
Copy link
Member Author

Tagging @dotnet/roslyn-ide
Note: tree diffing issue is reported here: #17498

@CyrusNajmabadi
Copy link
Member Author

retest windows_debug_unit32_prtest please

@CyrusNajmabadi
Copy link
Member Author

retest ubuntu_14_debug_prtest please

@CyrusNajmabadi
Copy link
Member Author

retest windows_debug_vs-integration_prtest please

@CyrusNajmabadi
Copy link
Member Author

retest windows_release_vs-integration_prtest please

@CyrusNajmabadi
Copy link
Member Author

retest ubuntu_16_debug_prtest please

Copy link
Contributor

@dpoeschl dpoeschl left a comment

Choose a reason for hiding this comment

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

Please make the comments clearer. Otherwise good.

{
var overlappingSpans = ArrayBuilder<TextChange>.GetInstance();
var intersectingSpans = ArrayBuilder<TextChange>.GetInstance();

var result = AllChangesCanBeApplied(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like calling things "result", even if it is very temporary.

}
else
var value = ChangeCanBeApplied(change,
overlappingSpans: overlappingSpans,
Copy link
Contributor

Choose a reason for hiding this comment

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

No named args needed.

ArrayBuilder<TextChange> intersectingSpans)
{
// Pure insertions (i.e. changes with empty spans) are complex to evaluate.
// We have to make sure that
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't leave me hanging!

ArrayBuilder<TextChange> overlappingSpans,
ArrayBuilder<TextChange> intersectingSpans)
{
// Pure insertions (i.e. changes with empty spans) are complex to evaluate.
Copy link
Contributor

Choose a reason for hiding this comment

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

"changes with empty overlapping/intersecting spans" or some other wording?

// And non-empty spans that we abut (i.e. which we're adjacent to) are totally safe
// for both to be applied.
//
// However, empty-span changes are extremely ambiguous. It is not possible to tell which
Copy link
Contributor

Choose a reason for hiding this comment

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

"empty-span changes" is a weird term. Can you just better define "pure insertion" at the beginning and then rely on that term instead?

@CyrusNajmabadi CyrusNajmabadi merged commit 43d12b6 into dotnet:master Mar 7, 2017
@CyrusNajmabadi CyrusNajmabadi deleted the fixAllWork2 branch March 7, 2017 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants