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

Use lazy evaluation for useMethodImpl #5728

Merged
merged 1 commit into from
Oct 11, 2018
Merged

Conversation

forki
Copy link
Contributor

@forki forki commented Oct 3, 2018

No description provided.

@KevinRansom
Copy link
Member

@forki
ci_part4 fails, can you check it out pls:

018-10-03T05:37:20.7984797Z Failed   Test request for parse and check doesn't check whole project
2018-10-03T05:37:20.7986109Z Error Message:
2018-10-03T05:37:20.7986946Z    Expected: true
2018-10-03T05:37:20.7987886Z Actual: false
2018-10-03T05:37:20.7988735Z   Expected: True
2018-10-03T05:37:20.7989523Z   But was:  False
2018-10-03T05:37:20.7990393Z 
2018-10-03T05:37:20.7991202Z Stack Trace:
2018-10-03T05:37:20.7992049Z    at FsUnit.shouldEqual[a](a x, a y) in D:\a\1\s\tests\service\FsUnit.fs:line 19
2018-10-03T05:37:20.7993231Z    at Tests.Service.ProjectAnalysisTests.Test request for parse and check doesn't check whole project() in D:\a\1\s\tests\service\ProjectAnalysisTests.fs:line 5099
2018-10-03T05:37:20.7994595Z 
2018-10-03T05:37:22.3161133Z Skipped  Test symbol uses of type-provided types
2018-10-03T05:38:01.3868710Z Results File: D:\a\1\s\fcs\FSharp.Compiler.Service.Tests\TestResults\VssAdministrator_factoryvm-az377_2018-10-03_05_33_36.trx
2018-10-03T05:38:01.3869574Z 
2018-10-03T05:38:01.3869790Z Total tests: 233. Passed: 212. Failed: 1. Skipped: 20.
2018-10-03T05:38:01.3871094Z Test Run Failed.
2018-10-03T05:38:01.3871926Z Test execution time: 4.5059 Minutes
2018-10-03T05:38:01.4622601Z 
2

@forki
Copy link
Contributor Author

forki commented Oct 3, 2018

Wow that's surprising.

@forki forki closed this Oct 3, 2018
@forki forki reopened this Oct 3, 2018
@KevinRansom
Copy link
Member

@forki,

I quite like this change, the code is uglier, but I think the logical short circuit evaluation will be beneficial.
It will take a bit more of a look to determine that the logic is the same, and probably a squint at the il to see what is generated.

Kevin

@forki
Copy link
Contributor Author

forki commented Oct 3, 2018

It's now green. I didn't change anything since that red build. I only restarted it since I was rather confident that it was same logic. So we must hit a flaky test or something.

@KevinRansom
Copy link
Member

@forki … agreed. I will take a look at the details over the weekend.

@KevinRansom
Copy link
Member

@forki

Thank you

@KevinRansom KevinRansom merged commit e42cea2 into dotnet:master Oct 11, 2018
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.

2 participants