Skip to content

Initial commit of dfdlx:DefineCharset - #1762

Open
jadams-tresys wants to merge 1 commit into
apache:mainfrom
jadams-tresys:DAFFODIL-2894
Open

jadams-tresys wants to merge 1 commit into
apache:mainfrom
jadams-tresys:DAFFODIL-2894

Conversation

@jadams-tresys

Copy link
Copy Markdown
Contributor

Allows for a user to define a simple character set directly inside a DFDL schema, without requiring any plugins.

Allows for a user to define a simple character set directly inside a
DFDL schema, without requiring any plugins.

@stevedlawrence stevedlawrence left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I really like this approach, this will definitely ease some pain points around custom characters sets. But I think there's room for simplification. Also some tests are failing, I think due to missing TDML support?

*/
private lazy val disallowedCharClassRegex = {
"(?:^|[^%])(?:%%)*(%(?:NL|ES|WSP|WSP\\+|WSP\\*);)".r
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we use a cooker for this instead of a custom regex? I believe we already have a cooker that with both disallow character classes and will also cook the charset string to their code points.

* The replacement character, used in a decodeString to mark an unassigned
* (unmappable) code point. Must match BitsCharsetNonByteSize.
*/
private final val UnicodeReplacementCharacter = '\uFFFD'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is already defined in CharsetUtils. I think it makes more sense to use that one since FFFD isn't really specific to DFDLDefineCharset

* <xs:annotation>
* <xs:appinfo source="http://www.ogf.org/dfdl/">
* <dfdlx:defineCharset
* name="BitsCharset6BitDFI408DUI505"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggest we don't use a DFI/DUI as an example. Few people are familar with these so it might be confusing. Maybe a simpler example is a hex encoding? I think that would look like:

<dfdlx:defineCharset
         name="Hexadecimal"
         bitWidthOfACodeUnit="4"
         decodeString="0123456789ABCDEF"
         replacementCharCode="0x0"
         requiredBitOrder="mostSignificantBitFirst" />

Or maybe the xample you ave is fine since it shows how to include tings like spaces. Maybe just change the name to remove the DFI/DUI.

* specification), e.g. %NUL;, %NBSP;, %#xHH;, %#DDD;, and %#rHH;, in addition
* to ordinary (XML-thawed) characters. The cooked decodeString must contain
* exactly 2**bitWidthOfACodeUnit characters, each with a Unicode code point
* less than 256. The U+FFFD (replacement character) marks an unassigned

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why restrict unicode code points to less than 256? For example in the Windows-1252 encoding, the byte 0x80 decodes to the euro symbol (€), which is \u20AC. Daffodil doesn't support Windows-1252, so if someone wanted to add this with defineCharset they would need the ability to specify code points >= 256

Comment on lines +207 to +208
<xs:attribute name="requiredBitOrder" type="dfdl:BitOrderEnum" default="mostSignificantBitFirst"/>
<xs:attribute name="mandatoryBitAlignment" type="xs:positiveInteger" default="1"/>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should both be required

<dfdlx:defineCharset name="UserASCII6BitMSB"
bitWidthOfACodeUnit="6"
replacementCharCode="0x0"
decodeString=" !&quot;#$%#x25;&amp;'()*+,-./0123456789:;&lt;=&gt;?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_"/>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sugestwe use %SP; for the space. technically a space character is legal, but it's more explicitly and avoids potential pretty printers messing things up.

limitations under the License.
-->

<tdml:testSuite

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to modify the tdml.xsd file to also reference dfdlx:defineCharset like it does for things like defineVariable and defineFormat? It would be nice to allow TDML files with embedded schemas to also use defineCharset. Woudl be nice to have tests showing this works too.

<tdml:parserTestCase name="sixBitUser" root="sixBitUser" model="DefineCharset.dfdl.xsd"
description="parse/unparse a user defined 6 bit charset, bit-for-bit equivalent to the predefined X-DFDL-6-BIT-DFI-264-DUI-001 charset">
<tdml:document>
<tdml:documentPart type="text" encoding="X-DFDL-6-BIT-DFI-264-DUI-001" bitOrder="LSBFirst"><![CDATA[0123456789 ABCDEFGHIJKLMNOPQRSTUVWXYZ012]]></tdml:documentPart>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is causing tests to fail, I don't think TDML tests have access to the schema defined encoders. And I think that's a reasonable limitation since it would probably be difficult to get access to compilation internals. I would suggest that these tests just need to specify the document part using type as bits or bytes

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh wait, this is a normal encoding supported by Daffodil. Nevermind, I'm not sure why these tests are failing them.

</tdml:errors>
</tdml:parserTestCase>

</tdml:testSuite>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be good to have a test showing that unicode replacement characters work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants