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.

Sign up for GitHub

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

Jump to bottom

[operators][2/2]feat: Add Assignment operation expr with xor assignemnt #342

Merged
merged 9 commits into from
Sep 28, 2020
Merged

[operators][2/2]feat: Add Assignment operation expr with xor assignemnt #342

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
92d0e7a
Implement xorAssignmentWith exprs with unit test
summer-ji-eng Sep 24, 2020
4a16e08
fix format
summer-ji-eng Sep 24, 2020
923c10f
fomart comment
summer-ji-eng Sep 24, 2020
d4d1d2d
Merge branch 'master' into xor_operator
summer-ji-eng Sep 24, 2020
290a6a4
update naming for Xor
summer-ji-eng Sep 24, 2020
47703a3
Merge branch 'xor_operator' of github.com:googleapis/gapic-generator-…
summer-ji-eng Sep 24, 2020
0406e66
simplify checking
summer-ji-eng Sep 25, 2020
e3b035e
fix import
summer-ji-eng Sep 25, 2020
b60aa5b
Merge branch 'master' into xor_operator
summer-ji-eng Sep 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/main/java/com/google/api/generator/engine/ast/AssignmentOperationExpr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ private AssignmentOperationExpr build() {
}

// Check type for XOR and assignment operator (^=).
// TODO(summerji): Complete the type-checking for ^= and unit test.
if (operator.equals(OperatorKind.ASSIGNMENT_XOR)) {
Preconditions.checkState(isValidXORAssignmentType(lhsType, rhsType), errorMsg);
}
Expand Down Expand Up @@ -130,9 +129,26 @@ private boolean isValidMultiplyAssignmentType(TypeNode variableType, TypeNode va
return false;
}

// TODO(summerji): Complete the type-checking for ^= and unit test.
// isValidXORAssignmentType validates the types for LHS variable expr and RHS expr.
// ^= can be applied on boolean and non-floating-point numeric type.
private boolean isValidXORAssignmentType(TypeNode variableType, TypeNode valueType) {
summer-ji-eng marked this conversation as resolved.
Show resolved Hide resolved
return true;
// LHS is boolean or its boxed type, RHS should be boolean or its boxed type.
if (variableType.equals(TypeNode.BOOLEAN)) {
return valueType.equals(variableType);
}
// LHS is non-floating-point numeric types, RHS should be non-float-point numeric types or
// their boxed types.
if (TypeNode.isNumericType(variableType)
&& !TypeNode.isBoxedType(variableType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider checking up-front and returning false first if:

  • One of variableType or valueType is not a numeric type.
  • valueType is boxed or is a floating-point type.
  • valueType is a floating-point type.

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 could do like

    private boolean isValidXorAssignmentType(TypeNode variableType, TypeNode valueType) {

      // LHS is boolean or its boxed type, RHS should be boolean or its boxed type.
      if (variableType.equals(TypeNode.BOOLEAN)) {
        return valueType.equals(variableType);
      }
      // LHS is integer boxed type, RHS should be non-floating-point numeric types or their boxed
      // types.
      if (variableType.equals(TypeNode.INT)) {
        return TypeNode.isNumericType(valueType) && !TypeNode.isFloatingPointType(valueType);
      }

      if (!TypeNode.isNumericType(variableType) || !TypeNode.isNumericType(valueType)) {
        return false;
      }
      if (TypeNode.isFloatingPointType(variableType) || TypeNode.isFloatingPointType(valueType)) {
        return false;
      }
      if (TypeNode.isBoxedType(variableType)) {
        return false;
      }
      return true;
    }

But I don't see the point of checking up-front. Could you explain more the benefit to do so?

Copy link
Contributor

Choose a reason for hiding this comment

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

When rearranged this way, we can now simplify it as follows:

return TypeNode.isNumericType(variableType) && TypeNode.isNumericType(valueType)
    && !TypeNode.isFloatingPointType(variableType) && !TypeNode.isFloatingPointType(valueType)
    && !TypeNode.isBoxedType(variableType);

The original benefit was to make the true-checks easier to read, but this rearrangement works too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, LMK what you think on new version.

&& !TypeNode.isFloatingPointType(variableType)) {
return TypeNode.isNumericType(valueType) && !TypeNode.isFloatingPointType(valueType);
}
// LHS is integer boxed type, RHS should be non-floating-point numeric types or their boxed
// types.
if (variableType.equals(TypeNode.INT)) {
return TypeNode.isNumericType(valueType) && !TypeNode.isFloatingPointType(valueType);
}
return false;
}
}
}
Loading