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

Clean up illib #4428

Merged
merged 6 commits into from
Mar 10, 2018
Merged

Clean up illib #4428

merged 6 commits into from
Mar 10, 2018

Conversation

forki
Copy link
Contributor

@forki forki commented Mar 4, 2018

No description provided.

@forki forki force-pushed the cleanup branch 3 times, most recently from 8a00ce1 to e26f462 Compare March 5, 2018 07:39
@forki forki changed the title [WIP Clean up illib Clean up illib Mar 5, 2018
@forki
Copy link
Contributor Author

forki commented Mar 5, 2018

done with this one

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Looks good to me; the whole tryDropPrefix stuff that didn't really do anything with the fact that it's optional seems like a good thing to clean up

@@ -1275,7 +1277,7 @@ module InfoMemberPrinting =
let paramDatas = minfo.GetParamDatas(amap, m, minst)
let layout =
layout ^^
if isNil (List.concat paramDatas) then
if Seq.isEmpty (Seq.concat paramDatas) then
Copy link
Contributor

@dsyme dsyme Mar 9, 2018

Choose a reason for hiding this comment

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

Please don't introduce Seq in the compiler unless there is a specific perf measurement, for much the same reasons as mentioned in other PR. I know there is an intermediate allocation, but ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it's not because of the intermediate allocation, but because it may will exit on very first item in paramDatas instead of concat and then throwing it away. But I can rewrite that as List.exists (not isNil) paramDatas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it's not because of the intermediate allocation, but because it may will exit on very first item in paramDatas instead of concat and then throwing it away. But I can rewrite that as List.exists (not isNil) paramDatas

@dsyme
Copy link
Contributor

dsyme commented Mar 9, 2018

Approved apart from the one thing i mentioned with Seq creeping in

@forki
Copy link
Contributor Author

forki commented Mar 10, 2018

Fixing a single line from mobile phone. How hard can it be?

@forki
Copy link
Contributor Author

forki commented Mar 10, 2018

Ok looks good now and much better than Seq.

@dsyme dsyme merged commit d7274c6 into dotnet:master Mar 10, 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.

3 participants