[GitHub] [daffodil] stevedlawrence commented on a change in pull request #581: Daffodil-2473 [WIP} XOR Operator

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #581:
URL: https://github.com/apache/daffodil/pull/581#discussion_r643044420



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DFDLXFunctions.scala
##########
@@ -98,6 +98,53 @@ case class DFDLXRightShift(recipes: List[CompiledDPath], argType: NodeInfo.Kind)
   }
 }
 
+case class DFDLXXor(recipes: List[CompiledDPath], argType1: NodeInfo.Kind,argType2:NodeInfo.Kind) extends FNTwoArgs(recipes) {
+  override def computeValue(arg1: DataValuePrimitive, arg2: DataValuePrimitive, dstate: DState): DataValuePrimitive = {
+
+    argType1 match {

Review comment:
       With the changes mentioned in previous comments you no longer need to pass in two types since they will always be the same. And so I think this logic all just becomes
   
   ```scala
   argType match {
     case NodeInfo.Long => arg1.getLong ^ arg2.getLong
     case NodeInfo.Int => arg1.getInt ^ arg2.getInt
     case NodeInfo.Short => (arg1.getShort ^ arg2.getShort).toShort
     ...
     case NodeInfo.UnsignedLong => arg1.getBigInt.xor(arg2.getBigInt)
     case NodeInfo.UnsignedInt => arg1.getLong ^ arg2.getLong
   }
   ```
   You don't have to do any nesting of match/cases because DPath will implicitly cast everything to the correct type.
   
   Some things worth pointing out:
   1. I don't think you need to do anything fancy for unsigned types like we had to do for the shifting functions. This is because the high order bits are all zeros for unsinged types and so we will always stay zero for the xor. So I think this can just use normal xor'slike you have
   1. For types that are reprsented by java primitives smaller than an Int (i.e. short, byte, unsignedByte), we need to make sure to cast the result to that type (e.g. short needs a toShort call). This is because in Java, the result of an xor always raises the type to an integer, but the result of the function needs to be the same as the original type. 
   

##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
##########
@@ -2177,7 +2180,42 @@ case class DFDLXShiftExpr(nameAsParsed: String, fnQName: RefQName,
     res
   }
 }
+case class DFDLXBitExpr(nameAsParsed: String, fnQName: RefQName,
+                        args: List[Expression], constructor: (List[CompiledDPath], NodeInfo.Kind,NodeInfo.Kind) => RecipeOp)
+  extends FunctionCallBase(nameAsParsed, fnQName, args) {
+  override lazy val inherentType = {
+    val argInherentType = args(0).inherentType
+    val arg2=args(1).inherentType
+    schemaDefinitionUnless((
+      argInherentType.isSubtypeOf(NodeInfo.PrimType.UnsignedLong) && arg2.isSubtypeOf(NodeInfo.PrimType.UnsignedLong))||  (argInherentType.isSubtypeOf(NodeInfo.PrimType.Long) && arg2.isSubtypeOf(NodeInfo.PrimType.Long)),
+      "First argument and Second argument both should  be either xs:unsignedLong  or a subtype of those or xs:long or a subtype of those",
+      nameAsParsed,argInherentType.globalQName)
+    argInherentType

Review comment:
       The `argInherentType` is just the type of the first argument, when we actually want it to by the type of which ever argument is larger. So we need logic that is something like:
   
   ```scala
   val arg0Type = args(0).inherentType
   val arg1Type = args(1).inherentType
   val argInherentType = if (arg1Type.isSubTypeOf(arg0Type)) arg0Type else arg1Type
   ```
   
   So if arg1 is a the same or smaller type as arg0, the function returns the type of arg0. Otherwise, it means arg1 is the larger type, and so the function returns the same type of arg1.
   
   This probably means we do want at least a couple tests where we make sure we try different types in both as both arg1 and arg2.

##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
##########
@@ -2177,7 +2180,42 @@ case class DFDLXShiftExpr(nameAsParsed: String, fnQName: RefQName,
     res
   }
 }
+case class DFDLXBitExpr(nameAsParsed: String, fnQName: RefQName,
+                        args: List[Expression], constructor: (List[CompiledDPath], NodeInfo.Kind,NodeInfo.Kind) => RecipeOp)
+  extends FunctionCallBase(nameAsParsed, fnQName, args) {
+  override lazy val inherentType = {
+    val argInherentType = args(0).inherentType
+    val arg2=args(1).inherentType
+    schemaDefinitionUnless((
+      argInherentType.isSubtypeOf(NodeInfo.PrimType.UnsignedLong) && arg2.isSubtypeOf(NodeInfo.PrimType.UnsignedLong))||  (argInherentType.isSubtypeOf(NodeInfo.PrimType.Long) && arg2.isSubtypeOf(NodeInfo.PrimType.Long)),
+      "First argument and Second argument both should  be either xs:unsignedLong  or a subtype of those or xs:long or a subtype of those",
+      nameAsParsed,argInherentType.globalQName)
+    argInherentType
+  }
+
 
+  override def targetTypeForSubexpression(subexp: Expression): NodeInfo.Kind = {
+    if (subexp == args(0))
+      inherentType
+    else if (subexp == args(1))
+      inherentType
+    else
+    //$COVERAGE-OFF$
+      Assert.invariantFailed("subexpression %s is not an argument.".format(subexp))
+    //$COVERAGE-ON$
+  }
+
+  override def compiledDPath: CompiledDPath = {
+    checkArgCount(2)
+    val arg0Recipe = args(0).compiledDPath
+    val arg0Type = args(0).inherentType
+    val arg1Type = args(1).inherentType
+    val arg1Recipe=args(1).compiledDPath
+    val c=conversions
+    val res = new CompiledDPath(constructor(List(arg0Recipe,arg1Recipe),arg0Type,arg1Type)+:c)

Review comment:
       I think using `args(0).inherentType` and `args(1).inherentType` is actually incorrect here. This is because `targetTypeForSubexpression` specifies that the target types for both subexpression is the same type. DPath will see what the expected target type is and add implicit casting. So for example, if we have an expression like this:
   
   ```
   dfdlx:xor(xs:long(5), xs:int(3))
   ```
   
   In this case, inherent type will be xs:long since it is the larger of the two. Additionally, targetTypeForSubexpression is going to return this inherent type for both args. Since the target type for both arguments is xs:long, DPath will automatically cast the second argument to an xs:long--you don't have to worry about it. So when DFDLXXor is called, you can be sure that both arguments have the same type, which is specified by inherentType. So this function probably wants to look something like this:
   
   ```scala
     checkArgCount(2)
     val argType = inherentType
     val arg0Recipe = args(0).compiledDPath
     val arg1Recipe = args(1).compiledDPath
     val c = conversions
     val res = new CompiledDPath(constructor(List(arg0Recipe,arg1Recipe),argType)+:c)
     res
   ```
   
   Note that we only pass in the single argType/inherentType to the constructor. This is because the types of the args will always be the same. This single type specifies what both arguments will be implicitly cast to, and what the xor function should return.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org