Skip to content

Conversation

Exanite
Copy link
Contributor

@Exanite Exanite commented Aug 21, 2025

This PR fixes #618 and some other issues that I identified as I was looking to understand the code.

#618 explains the two main cases fixed (missing type casts when remapping and incorrect sbyte case).

I also changed the uint/int case, mainly as an implementation detail. It doesn't matter if the cast happens before or after the left shift. It just has to happen before the right shift. This keeps the behavior consistent across cases.
Edit: I only changed the comment. The previous behavior already matches the current and the comment.

  // backing  int, current int            (value << cns) >> cns
  // backing  int, current uint           (uint)((value >> cns) & msk)

- // backing uint, current int            ((int)value << cns) >> cns
+ // backing uint, current int            (int)(value << cns) >> cns
  // backing uint, current uint           (value >> cns) & msk

  // backing uint, current byte           (byte)((value >> cns) & msk)
- // backing uint, current sbyte          (sbyte)((value << cns) >> cns)
+ // backing uint, current sbyte          (sbyte)((sbyte)(value << cns) >> cns)

You'll notice the code has changed a significant amount. This is because I rewrote most of the conditions and grouped the code together to make it a bit more readable. Because it was hard to track scopes, such as where each matching opening/closing parentheses were located, I added variables identifying the purpose and conditions for each scope.

Remaining tasks

  • Separate tests into Unix/Windows variants. I've tested on both platforms by manually accounting for the platform differences (different bitfield packing; different enum backing types), but I haven't added tests for both platforms yet.
  • Continue verifying that my new implementation works. It currently passes all test cases, but because of the larger changes made and how many conditions are required, I'm not yet confident that every case works.
    • Looks like CI tests failed. Windows tests failing is expected. Linux ARM tests failing is unexpected, but shouldn't be too hard to fix. I just need to research how C/C++ on Linux ARM behaves. Specifically, if char is a byte or sbyte on Linux ARM.
  • Update branch with changes from main

@Exanite
Copy link
Contributor Author

Exanite commented Aug 22, 2025

Ready for review.
All test cases pass.
Comparing current behavior with previous behavior shows that only the two targeted cases have been changed.

Since I changed the indentation, it's easier to see the changes if you ignore whitespace in your diff viewer.

@Exanite Exanite marked this pull request as ready for review August 22, 2025 22:41
var needsParenFirst = !isSmallType && isUnsignedToSigned;
var needsParenSecond = !needsParenFirst || isRemappedToSelf;
// Check if field/backing types match
var isTypeMismatch = type != typeBacking;
Copy link
Member

Choose a reason for hiding this comment

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

Why the comparison against typeBacking rather than buildinTypeBacking? Won't that be meaningful for certain typedefs or enums?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to take a look at this in more detail. I don't remember deciding against builtinTypeBacking, so it's likely something I missed when I was writing the condition. Changing typeBacking to builtinTypeBacking also doesn't affect the tests on either Linux nor Windows, so this probably is an untested case.

I'll draft the PR for now and undraft when I have an answer.

tannergooding
tannergooding previously approved these changes Sep 5, 2025
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just a minor nit and a question that I don't remember the answer to off the top of my head

Co-authored-by: Tanner Gooding <[email protected]>
@Exanite Exanite marked this pull request as draft September 6, 2025 00:16
This case is when typeBacking is not the same as builtinTypeBacking
@Exanite
Copy link
Contributor Author

Exanite commented Sep 6, 2025

Good catch. Using builtinTypeBacking instead of typeBacking is in fact important.
The difference with typeBacking is that an unnecessary type cast gets inserted.

I've added a test case and changed the code to use builtinTypeBacking instead.

@Exanite Exanite marked this pull request as ready for review September 6, 2025 01:58
@tannergooding tannergooding merged commit d431b90 into dotnet:main Sep 6, 2025
14 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.

Missing type casts when using bitfields with remapped types

2 participants