Scanner(unification)

[AdamSpeight2008]

Unification of methods

• TryScanToken
• ScanTokenFullWidth
• ScanToken_Unified

[mmitche]

This is failing with assertion popups. @jaredpar Can we avoid popup assertions in the tests?

[jaredpar]

@mmitche there are some Debug.Assert calls in this code and according to the following xUnit v2 stopped intercepting them hence it will popup a dialog again:

xunit/resharper-xunit#14

Is this blocking Jenkins or does it dismiss the dialogs after a time?

[AdamSpeight2008]

@jaredpar @mmitche The Debug.Assert already pre-existed in the Roslyn source.

@jaredpar it akin to doing a MessageBox.Show. How long do I have to wait for it to dismiss automatically?

[jaredpar]

@AdamSpeight2008 the Debug.Assert calls aren't firing on any of our machines. It's likely that it's a result of something that you've changed. Best path forward is to attach a debugger when it shows up and see why it is firing.

[AdamSpeight2008]

Still doesn't change that fact that Debug.Assert is causing a popup assertion.

[AnthonyDGreen]

@AdamSpeight2008,

It wouldn't matter whether the assert was a popup as it is now or whether it was trapped as by xUnit and treated as a unit test failure. The presentation has changed but the end result is that there has been a regression of some pre-existing assumption in compiler. Are all tests passing on your local machine?

[mmitche]

The result of this is that the test will hang until the popup is dismissed (this is not automatic). Aborting the test kills the popup, but that's not ideal.

[AnthonyDGreen]

What is _Exts? Did this folder exist beforehand?

[AdamSpeight2008]

Nope, I added it, it's name such to make it obvious is not a blessed namespace.
_Exts\Char

[AnthonyDGreen]

@mmitche, getting the Asserts to not popup is important but orthogonal. Even if there were no popups the test would still fail and we wouldn't be able to merge in the change. Whatever is causing the assert still has to be addressed.

[AdamSpeight2008]

@AnthonyDGreen True.
At the moment I can't figure it out. #dd49734596 should work as far as I can tell the unified method is correct, for the previous two methods.

[AnthonyDGreen]

@AdamSpeight2008,

Thanks for contributing! This PR includes several thousand changes. Many of which are formatting/style changes which aren't focused around the specific change you're making. Because we like to be pretty thorough in our reviews before accepting changes, particularly in areas as performance critical as the scanner the sheer volume of changes in prohibitive. It's not that the changes don't in some cases make the code more readable or more concise, but they make reviewing your core change several orders of magnitude more time consuming. Please revert these edits and constraint this PR to those changes germane to the unification you're suggesting so that the dev owners of that code can remain laser focused on changes with the highest impact and risk.

Additionally, please be more descriptive about the changes you're proposing and their motivation. I understand that there are some methods being unified but it's not immediately clear to us or others what's really being unified, how, and why. Some small code snippets demonstrating the before and after might be beneficial. Having a fairly descriptive overview of the PR is also critical to keeping the PR process as efficient as possible.

[pharring]

@AdamSpeight2008 Should this be closed now?

[AdamSpeight2008]

@pharring You can close it, when I get it to work I'll submit a new PR

[jaredpar]

Please do not submit a new PR. Instead reopen and reuse this one. It helps keep all of the feedback on a single logical change in the same location.

JaredPar from a phone
http://blog.paranoidcoding.com/

---

From: Adam Speight notifications@github.com
Sent: Thursday, March 5, 2015 2:55:00 PM
To: dotnet/roslyn
Cc: Jared Parsons
Subject: Re: [roslyn] Scanner(unification) (#782)

@pharringhttps://github.com/pharring You can close it, when I get it to work I'll submit a new PR

Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-77470434.

[citizenmatt]

The Debug.Assert dialog is default xunit behaviour. I think this should be fixed in xunit, so I've raised an issue: xunit/xunit#382