Skip to content

Fixed issue where text alignment was expected for enum#899

Merged
jadams-tresys merged 1 commit intoapache:mainfrom
jadams-tresys:DAFFODIL-2596
Dec 22, 2022
Merged

Fixed issue where text alignment was expected for enum#899
jadams-tresys merged 1 commit intoapache:mainfrom
jadams-tresys:DAFFODIL-2596

Conversation

@jadams-tresys
Copy link
Contributor

This small change checks against the repElement (the dfdx:repType) instead of the represented element in the enumeration (typically a string).

In the past we would typically expect the alignment to line up with whatever encoding was used for strings, but when using 1-bit alignment, daffodil wouuld complain about the strings being unaligned even though they are internally represented by something other than a string that is 1-bit aligned.

Now we check to see what kind of representation (text or binary) the repElement is and check alignment based on that.

DAFFODIL-2596

This small change checks against the repElement (the dfdx:repType)
instead of the represented element in the enumeration (typically a
string).

In the past we would typically expect the alignment to line up
with whatever encoding was used for strings, but when using 1-bit
alignment, daffodil wouuld complain about the strings being unaligned
even though they are internally represented by something other than a
string that is 1-bit aligned.

Now we check to see what kind of representation (text or binary) the
repElement is and check alignment based on that.

DAFFODIL-2596
@jadams-tresys
Copy link
Contributor Author

There may be other areas where repElement should be used in this code, but this definitely fixes the alignment issues that Mike B was seeing.

Copy link
Member

@stevedlawrence stevedlawrence left a comment

Choose a reason for hiding this comment

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

+1, but we really need to come up with a design where this kind of bug isn't possible. It wouldn't surprise me if there are other places where this kind of change is needed. And even if we track them all down, it's going to be too easy for new changes to not do this right.

Copy link
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

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

+1

@jadams-tresys jadams-tresys merged commit fef1284 into apache:main Dec 22, 2022
@jadams-tresys jadams-tresys deleted the DAFFODIL-2596 branch December 22, 2022 15:23
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.

3 participants