[GitHub] mbeckerle commented on a change in pull request #12: Revised daffodil-io module to require passing in a FormatInfo object.

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

mbeckerle commented on a change in pull request #12: Revised daffodil-io module to require passing in a FormatInfo object.
URL: https://github.com/apache/incubator-daffodil/pull/12#discussion_r159487266
 
 

 ##########
 File path: daffodil-core/src/main/scala/edu/illinois/ncsa/daffodil/grammar/BitOrderMixin.scala
 ##########
 @@ -87,29 +88,36 @@ trait BitOrderMixin extends GrammarMixin with ByteOrderAnalysisMixin { self: Ter
         (isArray && !hasUniformBitOrderThroughout)))
   }
 
-  //
-  //  protected final lazy val bitOrderChange = prod("bitOrderChange", needsBitOrderChange) {
-  //    BitOrderChange(this)
-  //  }
-
-  private lazy val checkByteAndBitOrder = {
-    val ev = new CheckByteAndBitOrderEv(termRuntimeData, defaultBitOrder)
-    ev.compile()
-    ev
+  lazy val maybeCheckByteAndBitOrderEv = {
+    //
+    // TODO: Performance: could be improved, as there are situations where byteOrder
+    // is defined, but still we know it will not be used and this could
+    // be Nope in those cases also. An example would be a 100% text-only item.
+    //
+    if (!isRepresented || !optionByteOrderRaw.isDefined)
 
 Review comment:
   Well the code base isn't entirely consistent on this. Some places try to keep in mind that technically, the DFDL spec doesn't require the byteOrder property for some formats - e.g., formats that are all text.  But all-text formats still have the notion of bitOrder - because charset encodings have their inherent bit order, and codepoints aren't always 1 byte wide.  So alignment fill is, in theory possible to be needed, and that requires bit order.
   
   Other parts of the code base punt on this, and just require byteOrder to be defined because it was expedient I suppose. 
   
   This code is trying to be true to the DFDL standard - if byteOrder isn't defined, there's no need to verify whether the byteOrder and bitOrder are compatible. The runtime just uses a maybe here because this check is optional. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services