-
Notifications
You must be signed in to change notification settings - Fork 167
Fix compile error when generating code involving unsigned int bitshifts #612
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 compile error when generating code involving unsigned int bitshifts #612
Conversation
This is to fix: dotnet#611 Always casting to int will work because C# only allows ints as the RHS parameter for bitshifts: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#1211-shift-operators
c1820e7
to
86c1d97
Compare
tests/ClangSharp.PInvokeGenerator.UnitTests/Base/FunctionDeclarationBodyImportTest.cs
Show resolved
Hide resolved
// RHS of shift operation in C# must be an int | ||
outputBuilder.Write("(int)"); | ||
outputBuilder.Write('('); | ||
|
||
Visit(binaryOperator.RHS); | ||
|
||
outputBuilder.Write(')'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
…always casting to int
86c1d97
to
8ac6d65
Compare
…and non-literal integer expressions
if (binaryOperator.RHS.Type.Kind is CXType_Int or CXType_Long) | ||
{ | ||
Visit(binaryOperator.RHS); | ||
} |
There was a problem hiding this comment.
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.
else if (binaryOperator.RHS is IntegerLiteral literal) | ||
{ | ||
outputBuilder.Write(literal.Value); | ||
} |
There was a problem hiding this comment.
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.
Mostly looks right, just a couple of edge cases that I think were missed |
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.