Skip to content

Conversation

Exanite
Copy link
Contributor

@Exanite Exanite commented Jul 10, 2025

This fixes #611.

This changes it so that the right-hand side of bitshifts always gets casted to an int.
This works because C# only accepts ints as the RHS of bitshifts: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#1211-shift-operators

Note: This leads to widespread changes in how the bitshift operator is output (see test cases). If necessary, I can look into a less invasive approach. The reasoning for doing this globally is to keep things consistent and the extra cast shouldn't affect anything other than adding a bit of bloat to the generated code.

This now casts only if the RHS is a non-literal, non-int expression. Otherwise, the literal or expression is used directly.

@Exanite Exanite force-pushed the fix/unsigned-int-bitshift branch 2 times, most recently from c1820e7 to 86c1d97 Compare July 10, 2025 11:02
@Exanite Exanite marked this pull request as ready for review July 10, 2025 11:09
Comment on lines 47 to 53
// RHS of shift operation in C# must be an int
outputBuilder.Write("(int)");
outputBuilder.Write('(');

Visit(binaryOperator.RHS);

outputBuilder.Write(')');
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't do this so that it always casts. Rather, I would attempt to handle some common cases like constant integers and values that are already of the right type to not need the cast

We should really only cast if it is an unknown variable of type uint, long, ulong, etc.

Copy link
Member

Choose a reason for hiding this comment

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

There are various helpers like GetExprAsWritten, IsUnchecked, IsUnsigned, IsStmtAsWritten, and a few others that are used to do similar things in other places.

You can see how this works with cases like UncheckStmt: https://source.clangsharp.dev/#ClangSharp.PInvokeGenerator/PInvokeGenerator.cs,16fa21ae00284ca6,references, VisitCharacterLiteral: https://source.clangsharp.dev/#ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitStmt.cs,f623229c061260b9,references, VisitImplicitCastExpr: https://source.clangsharp.dev/#ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitStmt.cs,14c51a7fef97ef30,references, etc

All of these have similar considerations (and comments) around how C/C++ handle things vs how C# does and where various casts need to be inserted vs where they can be avoided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review and pointers! I'll take a look

@Exanite Exanite force-pushed the fix/unsigned-int-bitshift branch from 86c1d97 to 8ac6d65 Compare July 12, 2025 07:44
@Exanite Exanite marked this pull request as draft July 12, 2025 08:00
@Exanite Exanite marked this pull request as ready for review July 12, 2025 08:11
@Exanite Exanite requested a review from tannergooding July 12, 2025 08:13
@Exanite Exanite marked this pull request as draft July 12, 2025 20:40
@Exanite Exanite marked this pull request as ready for review July 12, 2025 21:47
Comment on lines 48 to 51
if (binaryOperator.RHS.Type.Kind is CXType_Int or CXType_Long)
{
Visit(binaryOperator.RHS);
}
Copy link
Member

Choose a reason for hiding this comment

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

Notably checking for CXType_Long isn't correct. This is only int on Windows and is nint on Unix.

I'm also not sure this will do the "right" thing for something like x << (byte)(5) or will it end up producing x << (int)((byte)(5))?

It's really only large types that need the cast.

Comment on lines 52 to 55
else if (binaryOperator.RHS is IntegerLiteral literal)
{
outputBuilder.Write(literal.Value);
}
Copy link
Member

Choose a reason for hiding this comment

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

This won't do the right thing for an integer literal whose value is actually larger than 32-bits. Which while unlikely, should probably be handled.

@tannergooding
Copy link
Member

Mostly looks right, just a couple of edge cases that I think were missed

@Exanite Exanite marked this pull request as draft July 18, 2025 15:08
@Exanite
Copy link
Contributor Author

Exanite commented Jul 19, 2025

I think I got most of the cases.
The tested output all compiles:
image

I still need to check how the generator behaves when GenerateDisableRuntimeMarshalling and/or GenerateUnixTypes is set to true.

There are also a few unideal cases that require more work than it is probably worth.
I note these in the test cases:

// Non-ideal cases:
// UByte
// IntMin
// ULongMax
// Hexadecimal

Note that the unchecked is getting added by some other part of the generator. I did not modify that behavior.

@Exanite Exanite marked this pull request as ready for review July 20, 2025 04:07
@Exanite Exanite requested a review from tannergooding July 20, 2025 04:07
@tannergooding tannergooding merged commit 4d4e062 into dotnet:main Jul 21, 2025
13 checks passed
@Exanite Exanite deleted the fix/unsigned-int-bitshift branch September 2, 2025 14:27
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.

ClangSharpPInvokeGenerator does not handle unsigned int bitshifts correctly

2 participants