Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bytes type #50

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Fix bytes type #50

merged 1 commit into from
Nov 14, 2023

Conversation

arg0d
Copy link
Contributor

@arg0d arg0d commented Nov 13, 2023

Type::Bytes produces incorrect code in the following situations:

  • u8 is not referenced anywhere else in the definition file. This causes undefined symbol FfiConverterUint8.
  • Vec<u8> is referenced somewhere in the definition file. This causes duplicate symbol definition for FfiConverterSequenceUint8.

To fix this, replace Type::Bytes with Type::Sequence<u8> in known types set before rendering Type.cs. This ensures that both FFI converters FfiConverterSequenceUInt8 and FfiConverterUInt8 will be rendered exactly once in Types.cs.

Fixes #49.

@arg0d arg0d requested a review from Lipt0nas November 13, 2023 12:38
Copy link
Member

@Lipt0nas Lipt0nas left a comment

Choose a reason for hiding this comment

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

Look good

Just one question - since the type is being tested in the already present coverall tests, do we actually need a whole separate test that functionally does the same thing?

@arg0d
Copy link
Contributor Author

arg0d commented Nov 14, 2023

The separate tests ensures that bytes type is tested in isolation without Vec<u8 and u8 types being used in UDL. Testing reverse itself is not necessary, just have to make sure the code compiles. I will move the comment from UDL file to test file.

`Type::Bytes` produces incorrect code in the following situations:
- `u8` is not referenced anywhere else in the definition file. This
    causes undefined symbol `FfiConverterUint8`.
- `Vec<u8>` is referenced somewhere in the definition file. This causes
    duplicate symbol definition for `FfiConverterSequenceUint8`.

To fix this, replace `Type::Bytes` with `Type::Sequence<u8>` in known
types set before rendering `Type.cs`. This ensures that both FFI
converters `FfiConverterSequenceUInt8` and `FfiConverterUInt8` will
be rendered exactly once in `Types.cs`.
@arg0d arg0d force-pushed the LLT-4526_fix-bytes-type branch from 1d198e0 to 81bf26b Compare November 14, 2023 09:19
@arg0d arg0d merged commit dac800c into main Nov 14, 2023
4 checks passed
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.

bytes type produces incorrect code in some situations
2 participants