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

No need to decompile coreDisplayName all the time #4443

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

forki
Copy link
Contributor

@forki forki commented Mar 6, 2018

No description provided.

@forki forki closed this Mar 6, 2018
@forki forki reopened this Mar 6, 2018
if (idRange.EndColumn - idRange.StartColumn <= 5) &&
not cenv.g.compilingFslib
then
let opName = DecompileOpName coreDisplayName
Copy link
Member

Choose a reason for hiding this comment

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

Not every branch of the pattern uses DecompileOpName.
With this change, it will happen regardless of the branch executed. Im guessing that it will now be called more times.
I suppose you can make it lazy with: let opName () = DecompileOpName coreDisplayName

Kevin

Copy link
Member

Choose a reason for hiding this comment

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

Okay upon closer look, this looks good. Ignore my prvious comment.

match opName with
| PrettyNaming.Relational ->
if isMember then
warning(StandardOperatorRedefinitionWarning(FSComp.SR.tcInvalidMethodNameForRelationalOperator(opName, (CompileOpName opName)), idRange))
if Option.isSome memberInfoOpt then
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we bring this up? ex: let isMember = Option.isSome memberInfoOpt and use isMember

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer memberInfoOpt.IsSome to Option.isSome memberInfoOpt. Use properties if they are there

@forki forki force-pushed the CheckForAbnormalOperatorNames branch 2 times, most recently from 3b0ac24 to 07d31bc Compare March 8, 2018 07:02
@forki
Copy link
Contributor Author

forki commented Mar 8, 2018

done.

@forki forki force-pushed the CheckForAbnormalOperatorNames branch from 07d31bc to 29e9ad6 Compare March 8, 2018 07:15
@KevinRansom
Copy link
Member

Thanks for this.

@KevinRansom KevinRansom merged commit ed8393c into dotnet:master Mar 9, 2018
KevinRansom pushed a commit that referenced this pull request Mar 9, 2018
* One lookup in nenv.eFieldLabels should be enough (#4469)

* Shortcut NameResolution searches (#4444)

* No need to decompile coreDisplayName all the time (#4443)
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.

4 participants