Sizeof support #284
Sizeof support #284
Conversation
WalkthroughThe recent changes enhance the functionality of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Compiler
participant ILGenerator
participant TypeChecker
User->>Compiler: Request to compile code
Compiler->>TypeChecker: Determine types in code
TypeChecker->>TypeChecker: Check for SizeOfFunctionExpression
TypeChecker-->>Compiler: Return TypeCode
Compiler->>ILGenerator: Emit IL code based on types
ILGenerator->>ILGenerator: Handle null checks and emit size
ILGenerator-->>Compiler: Return emitted IL
Compiler-->>User: Compilation complete
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (3)
runtime/ishtar.generator/generators/emitters.cs (3)
77-101: Consider clarifying the error message for generics.The error message for using
sizeofwith generics could be more informative. Consider specifying that the limitation is temporary and will be addressed in future updates.- context.LogError($"sizeof cannot use with generics [limitation is temporary, awaiting natural structs and fully generics implementation].", sizeOf); + context.LogError($"'sizeof' cannot be used with generic types. This limitation is temporary and will be resolved with future support for natural structs and full generics.", sizeOf);
102-124: Enhance error message specificity for missing type arguments.The error message for missing type arguments could specify that a single type argument is required.
- context.LogError($"as type required type argument", nameOf); + context.LogError($"'typeof' requires a single type argument.", nameOf);
145-154: Enhance error message specificity for missing type arguments.The error message for missing type arguments could specify that a single type argument is required.
- context.LogError($"type is required single type argument", typeOf); + context.LogError($"'type is' requires a single type argument.", typeOf);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- runtime/common/reflection/VeinTypeCode.cs (1 hunks)
- runtime/ishtar.generator/generators/emitters.cs (3 hunks)
- runtime/ishtar.generator/generators/operators.cs (2 hunks)
- runtime/ishtar.generator/generators/types.cs (1 hunks)
- test/vc_test/Features/EtherealFunctionTest.cs (2 hunks)
- test/vc_test/FullSTDCompilation.cs (3 hunks)
Additional comments not posted (7)
test/vc_test/Features/EtherealFunctionTest.cs (2)
19-21: LGTM!The
SizeOfTestmethod correctly tests the parsing of thesizeof<bool>()expression using theSyntax.ethereal_function_expressionmethod.
38-38: Test case addition approved.The addition of the
sizeoftest case to theAllmethod enhances the test coverage for thesizeof<Type>()expression.runtime/common/reflection/VeinTypeCode.cs (1)
225-226: LGTM!The
HasObjectOrClassmethod correctly identifiesVeinTypeCodeinstances that represent either a class or an object.runtime/ishtar.generator/generators/emitters.cs (1)
72-73: Addition ofSizeOfFunctionExpressionhandling is appropriate.The inclusion of
SizeOfFunctionExpressionenhances the macro emission capabilities by supporting size calculations.runtime/ishtar.generator/generators/types.cs (1)
194-195: Support forSizeOfFunctionExpressionis well-integrated.The handling of
SizeOfFunctionExpressioninDetermineTypeappropriately returnsVeinTypeCode.TYPE_I4, aligning with the expected behavior for size calculations.runtime/ishtar.generator/generators/operators.cs (2)
55-56: Enhancements to handle null types in binary expressions are effective.The added logic in
EmitBinaryExpressionto handle null types improves robustness and aligns with expected behavior for object and class comparisons.
86-87: Introduction ofIsNullTypeutility method is beneficial.The
IsNullTypemethod provides a clear and concise way to check for null types, enhancing code readability and maintainability.
| @@ -55,7 +55,7 @@ public void Compile() | |||
| AppFlags.Set(ApplicationFlag.use_predef_array_type_initer, true); | |||
| var project = VeinProject.LoadFrom(project_file); | |||
| var settings = new CompileSettings(); | |||
| var targets = CompilationTask.Run(project.WorkDir, settings); | |||
| var targets = CompilationTask.RunAsync(project.WorkDir, settings).Result; | |||
There was a problem hiding this comment.
Consider using await instead of .Result for asynchronous calls.
Using .Result on an asynchronous method can lead to blocking and potential deadlocks. Consider using await to handle the asynchronous call more effectively, assuming the method can be made asynchronous.
- var targets = CompilationTask.RunAsync(project.WorkDir, settings).Result;
+ var targets = await CompilationTask.RunAsync(project.WorkDir, settings);Note: This change would require the method to be marked as async and return a Task.
Committable suggestion was skipped due to low confidence.
| var context = gen.ConsumeFromMetadata<GeneratorContext>("context"); | ||
|
|
||
| // TODO, validate exist variable\field\method\class\etc | ||
| if (nameOf.Expression is AccessExpressionSyntax nameof_exp1) | ||
| if (!nameOf.Expression.IsDefined) | ||
| { | ||
| context.LogError($"nameof required single type argument", nameOf); | ||
| throw new SkipStatementException(); | ||
| } | ||
| if (nameOf.Expression.Get() is AccessExpressionSyntax nameof_exp1) | ||
| return gen.Emit(OpCodes.LDC_STR, nameof_exp1.Right.ToString()); | ||
| if (nameOf.Expression is IdentifierExpression nameof_exp2) | ||
| if (nameOf.Expression.Get() is IdentifierExpression nameof_exp2) | ||
| return gen.Emit(OpCodes.LDC_STR, nameof_exp2.ToString()); | ||
| var ctx = gen.ConsumeFromMetadata<GeneratorContext>("context"); | ||
| ctx.LogError($"Target expression is not valid named expression.", nameOf); | ||
| context.LogError($"Target expression is not valid named expression.", nameOf); |
There was a problem hiding this comment.
Reminder: Complete validation for EmitNameOf.
The TODO comment indicates that additional validation is needed for variable, field, method, and class existence.
Do you want me to help implement this validation or open a GitHub issue to track this task?
Summary by CodeRabbit
New Features
HasObjectOrClassandIsNullType.SizeOfFunctionExpressionin type determination and IL code emission.Bug Fixes
Tests
sizeoffunctionality to ensure accurate parsing and validation.Chores