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

Added VB Operation Factory #19073

Merged
merged 5 commits into from
May 2, 2017

Conversation

heejaechang
Copy link
Contributor

added VB Operation Factory.

@heejaechang heejaechang added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 28, 2017
Copy link
Contributor

@AdamSpeight2008 AdamSpeight2008 left a comment

Choose a reason for hiding this comment

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

Could we at least have an empty line between the methods?

@heejaechang
Copy link
Contributor Author

@AdamSpeight2008 style can be done later.

@heejaechang heejaechang removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 28, 2017
@heejaechang
Copy link
Contributor Author

@dotnet/roslyn-analysis can you take a look? @dotnet/analyzer-ioperation

End Function
Private Shared Function CreateBoundCallOperation(boundCall As BoundCall) As IInvocationExpression
Dim targetMethod As IMethodSymbol = boundCall.Method
Dim instance As Lazy(Of IOperation) = New Lazy(Of IOperation)(Function() If(boundCall.Method.IsShared, Nothing, DirectCast(Create(boundCall.ReceiverOpt), IOperation)))
Copy link
Contributor

Choose a reason for hiding this comment

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

   Dim instance As New Lazy(Of IOperation)(Function() If(boundCall.Method.IsShared, Nothing, DirectCast(Create(boundCall.ReceiverOpt), IOperation)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unnecessary cast is there since code is auto-generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's it is producing sub-optimal stylistic code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do style only clean up PR next time.


Private Shared Function GetChildOfBadExpression(parent As BoundNode, index As Integer) As IOperation
Dim badParent As BoundBadExpression = TryCast(parent, BoundBadExpression)
If badParent IsNot Nothing AndAlso badParent.ChildBoundNodes.Length > index Then
Copy link
Contributor

Choose a reason for hiding this comment

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

     If badParent ?.ChildBoundNodes.Length > index Then

For Each memberAssignment In objectInitializerExpression.Initializers
Dim assignment = TryCast(memberAssignment, BoundAssignmentOperator)
Dim left = assignment?.Left
If left IsNot Nothing Then
Copy link
Contributor

Choose a reason for hiding this comment

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

If left Is Nothing The Continue For

Copy link
Contributor

Choose a reason for hiding this comment

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

If left Is Nothing The Continue For

We prefer block-if to a line-if

Return arrayType.ElementType
End If

Return Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Return arrayType?.ElementType

Return clause.OperandOpt
End If

If clause.ConditionOpt IsNot Nothing AndAlso clause.ConditionOpt.Kind = BoundKind.BinaryOperator Then
Copy link
Contributor

Choose a reason for hiding this comment

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

If clause.ConditionOpt?.Kind = BoundKind.BinaryOperator Then


Dim stepValue As BoundExpression = boundFor.StepValue
If stepValue Is Nothing Then
stepValue = New BoundLiteral(Nothing, Semantics.Expression.SynthesizeNumeric(controlType, 1), controlType)
Copy link
Contributor

Choose a reason for hiding this comment

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

stepValue = If( stepValue, New BoundLiteral(Nothing, Semantics.Expression.SynthesizeNumeric(controlType, 1), controlType))

@heejaechang
Copy link
Contributor Author

@AlekseyTs @cston @jcouv can you take a look?

@jinujoseph
Copy link
Contributor

@dotnet/roslyn-compiler , @AnthonyDGreen

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

Optimistically LGTM under assumption that the existing code was simply moved.

@heejaechang
Copy link
Contributor Author

@dotnet/roslyn-compiler can I get one more compiler dev sign off?

@@ -172,7 +172,7 @@ Namespace Microsoft.CodeAnalysis.Semantics
Dim kind = GetAssignmentKind(boundAssignmentOperator)
If kind = OperationKind.CompoundAssignmentExpression Then
Dim binaryOperationKind As BinaryOperationKind = DirectCast(Create(boundAssignmentOperator.Right), IBinaryOperatorExpression).BinaryOperationKind
Dim target As Lazy(Of IOperation) = New Lazy(Of IOperation)(Function() DirectCast(Create(boundAssignmentOperator.Left), IOperation))
Dim target As Lazy(Of IOperation) = New Lazy(Of IOperation)(Function() Create(boundAssignmentOperator.Left))
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the code that as the form

  Dim target As Lazy(Of IOperation) = New Lazy(Of IOperation)(Function() Create(boundAssignmentOperator.Left))

can be changed to,

  Dim target As New Lazy(Of IOperation)(Function() Create(boundAssignmentOperator.Left))

without affecting the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now, I will just leave the code as it is.

@cston
Copy link
Member

cston commented May 2, 2017

LGTM

@heejaechang heejaechang merged commit 4596238 into dotnet:features/ioperation May 2, 2017
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.

6 participants