[GitHub] [daffodil] mbeckerle commented on a diff in pull request #998: Add left over data check to generated C parsers

["mbeckerle (via GitHub)" <gi...@apache.org>]

mbeckerle commented on code in PR #998:
URL: https://github.com/apache/daffodil/pull/998#discussion_r1156145143


##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/unparsers.c:
##########
@@ -465,3 +465,30 @@ unparse_check_bounds(const char *name, size_t count, size_t minOccurs, size_t ma
         ustate->error = &error;
     }
 }
+
+// Flush any fractional bits not written yet
+
+void
+flush_ustate(UState *ustate)

Review Comment:
   Hmmm. It's a fraction of a byte yes? Not sure what a fractional bit is. I get that you mean one of the bits of the fractional byte, but I would call it flush_fractional_byte. 



##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/parsers.c:
##########
@@ -594,3 +594,34 @@ parse_check_bounds(const char *name, size_t count, size_t minOccurs, size_t maxO
         pstate->error = &error;
     }
 }
+
+// Check for any data left over after end of parse
+
+void
+check_pstate(PState *pstate)
+{
+    // Skip the check if we already have an error
+    if (!pstate->error)
+    {
+        // Check for any unread bits left in pstate's fractional byte
+        if (pstate->unreadLen)

Review Comment:
   Consider maybe renaming unreadLen to unreadBitsLen to make it clear this is in units of bits.
   
   Since most library and other C code works in bytes I think it's useful to specify Bits when that's the units. 
   



##########
daffodil-codegen-c/src/test/examples/NestedUnion/generated_code.c:
##########
@@ -94,7 +94,7 @@ static const ERD foo_data_NestedUnionType_ERD = {
     },
     COMPLEX, // typeCode
     3, // numChildren
-    foo_data_NestedUnionType__offsets, // offsets
+    foo_data_NestedUnionType__childrenOffsets, // childrenOffsets

Review Comment:
   comments seem redundant with identifier names. Here and also in a few other spots below. 
   
   The comments add value for the things that aren't identifiers that end in exactly a suffix that matches the comment.  But for the ones where there is an identifiers, they really are just clutter. 



##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/generators/CodeGeneratorState.scala:
##########
@@ -400,7 +427,7 @@ class CodeGeneratorState(private val root: ElementBase) {
       if (numChildren > 0)
         s"""static const $C ${C}_compute_offsets;
          |
-         |static const size_t ${C}_offsets[$count] = {
+         |static const size_t ${C}_childrenOffsets[$count] = {

Review Comment:
   You didn't snake-case numerous identifiers like childrenOffsets, so I assume that change was not intended to be pervasive for all identifiers. 



##########
daffodil-codegen-c/src/main/resources/org/apache/daffodil/codegen/c/files/libruntime/parsers.c:
##########
@@ -594,3 +594,34 @@ parse_check_bounds(const char *name, size_t count, size_t minOccurs, size_t maxO
         pstate->error = &error;
     }
 }
+
+// Check for any data left over after end of parse
+
+void
+check_pstate(PState *pstate)

Review Comment:
   Is this going to generalize to other checks of the pstate? If it's is just about the left-over-data, then rename accordingly. 
   
   Also consider since this is about after the end of the parse, why  not "check_final_pstate" to reflect this is not an internal consistency check, but for checking at the end. 



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

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