Fixes/branch logic improvements #239
Conversation
0xF6
added
area-vm
WalkthroughThe changes consist of various enhancements and optimizations across multiple files. Key modifications include updates to method signatures, additions of new parameters, adjustments in control flow, improved error handling, debugging aids, and refactoring of class constructors and initializations. These alterations aim to improve code clarity, efficiency, and maintainability while introducing new features and settings for optimization control. Changes
Poem
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 Configration 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: 13
Outside diff range and nitpick comments (7)
runtime/ishtar.generator/VeinScope.cs (1)
15-15: Add documentation forScopeId.It would enhance understanding if there were a comment explaining the purpose of
ScopeIdand how it is used withinVeinScope.compiler/Log.cs (1)
Line range hint
55-78: Improve error reporting format.The method
_printcould benefit from a more structured format for error messages, especially when bothposedanddocare not null.- var diff_err = posed.Transform.DiffErrorFull(doc); - strBuilder.Append($"\t\t{diff_err}"); + if (posed != null && doc != null) + { + var diff_err = posed.Transform.DiffErrorFull(doc); + strBuilder.AppendLine("\tDetails:"); + strBuilder.AppendLine($"\t\tFile: {doc.FullName}"); + strBuilder.AppendLine($"\t\tError: {diff_err}"); + }runtime/ishtar.vm/vm.shit.cs (1)
66-67: Clarify the purpose of_trueand_falseconstants.It's good practice to add comments explaining the purpose of these constants, especially since they are pivotal in the comparison logic.
runtime/ishtar.base/emit/ILGenerator.cs (3)
Line range hint
355-430: Refactor exception handling blocks for clarity and robustness.The methods
BeginTryBlock,EndExceptionBlock, andBeginFinallyBlockcould be refactored to improve readability and error handling robustness. Consider encapsulating some of the logic into smaller, more focused methods.
Line range hint
453-479: Improve label management in theDefineLabelandUseLabelmethods.These methods could benefit from a more robust management system for labels, potentially using a dedicated class or structure to handle label storage and retrieval, which could improve maintainability and error prevention.
Line range hint
393-393: Address the nullable reference type annotation issue.Ensure that nullable reference type annotations are used within a '#nullable' annotations context to comply with C# best practices.
runtime/ishtar.vm/VirtualMachine.cs (1)
Line range hint
1-1131: General review: Error handling, performance, and consistency.This file is critical for the performance and reliability of the virtual machine. It is recommended to:
- Review all exception handling paths to ensure they are robust and cover all possible error conditions.
- Optimize memory management, particularly in methods that allocate significant resources.
- Ensure consistency in the handling of different data types and operations across the entire file.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- compiler/Log.cs (3 hunks)
- compiler/cmd/CompileCommand.cs (6 hunks)
- compiler/compilation/CompilationTask.cs (3 hunks)
- compiler/compilation/parts/bodies.cs (1 hunks)
- compiler/veinc.csproj (1 hunks)
- lib/ast/stl/VeinParserExtensions.cs (3 hunks)
- lib/ast/syntax/ErrorDiff.cs (3 hunks)
- lib/ast/syntax/PreviewParseExtension.cs (1 hunks)
- lib/ast/syntax/ast/expressions/functions/CorruptedChainException.cs (1 hunks)
- runtime/ishtar.base/emit/ILGenerator.cs (6 hunks)
- runtime/ishtar.base/emit/Label.cs (1 hunks)
- runtime/ishtar.generator/GeneratorContext.cs (2 hunks)
- runtime/ishtar.generator/VeinScope.cs (4 hunks)
- runtime/ishtar.generator/generators/cycles.cs (1 hunks)
- runtime/ishtar.generator/generators/logic.cs (2 hunks)
- runtime/ishtar.generator/generators/operators.cs (3 hunks)
- runtime/ishtar.generator/generators/types.cs (1 hunks)
- runtime/ishtar.vm/VirtualMachine.cs (3 hunks)
- runtime/ishtar.vm/stackval.cs (2 hunks)
- runtime/ishtar.vm/vm.shit.cs (1 hunks)
Files skipped from review due to trivial changes (1)
- compiler/compilation/parts/bodies.cs
Additional context used
GitHub Check: build_all (ubuntu-latest, linux-x64, true)
lib/ast/syntax/ErrorDiff.cs
[failure] 25-25:
The type or namespace name 'FileInfo' could not be found (are you missing a using directive or an assembly reference?)runtime/ishtar.vm/stackval.cs
[warning] 9-9:
The type name 'stackval' only contains lower-cased ascii characters. Such names may become reserved for the language.runtime/ishtar.base/emit/ILGenerator.cs
[warning] 393-393:
The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
GitHub Check: Codacy Static Code Analysis
runtime/ishtar.vm/vm.shit.cs
[notice] 59-59: runtime/ishtar.vm/vm.shit.cs#L59
Remove these redundant parentheses.
Additional comments not posted (20)
runtime/ishtar.base/emit/Label.cs (2)
7-7: Proper use of internal access modifier forValueensures encapsulation within the assembly.
8-8: Good encapsulation with internal access forName, aligns with the encapsulation strategy ofValue.lib/ast/syntax/ast/expressions/functions/CorruptedChainException.cs (1)
6-7: Excellent use of contextual information in the exception message, enhancing the clarity and utility of error reporting.runtime/ishtar.generator/generators/cycles.cs (2)
13-14: Proper use of descriptive labels enhances readability and debuggability of the loop constructs.Also applies to: 17-17
17-17: Good practice to check the expression type to ensure it's boolean before evaluating it in a conditional context. Prevents runtime type errors.compiler/veinc.csproj (1)
6-7: Enabling implicit usings and nullable reference types are modern C# practices that enhance code quality and safety.runtime/ishtar.vm/stackval.cs (2)
8-8: Adding the DebuggerDisplay attribute enhances the debugging experience by providing a clear, custom string representation ofstackvalinstances.
80-107: The expanded ToString method now handles all VeinTypeCode types, providing a comprehensive and informative string representation for each type.lib/ast/syntax/PreviewParseExtension.cs (1)
56-85: The refactoredPreviewMultiplemethod improves readability and functionality by clearly handling multiple parsers and outcomes. It also optimizes the process of determining the best parsing result.lib/ast/stl/VeinParserExtensions.cs (2)
64-64: Enhancing theVeinParseExceptionconstructor to include detailed position information improves error reporting and diagnostics.
119-121: The introduction of theExchangeWrapperstruct simplifies parser transformations, enhancing modularity and reusability of parsing components.compiler/cmd/CompileCommand.cs (2)
24-27: Adding theDisableOptimizationcommand option provides users with more control over the compilation process by allowing them to disable optimizations if needed.
Line range hint
55-161: The refactored command execution logic inCompileCommandand related classes improves handling of multiple projects and enhances error reporting, making the compilation process more robust and user-friendly.runtime/ishtar.generator/GeneratorContext.cs (1)
Line range hint
14-48: Proper implementation of configuration and scope management.The introduction of
GeneratorContextConfigfor managing configurations and the changes in scope initialization are well-implemented.compiler/compilation/CompilationTask.cs (2)
Line range hint
25-39: Well-structured constructor and property initialization.The restructuring of the
CompilationTaskconstructor to include initialization of properties directly from parameters enhances clarity and maintainability.
209-211: Correct setup of context and module initialization.The initialization of the
ContextwithGeneratorContextConfigand the setup of themoduleare correctly implemented to align with the new configuration strategy.runtime/ishtar.generator/generators/operators.cs (3)
31-31: Enhanced handling of binary expressions.The updated handling of binary expressions to include both numeric and non-numeric types, along with improved error handling, is commendable.
239-239: Improved unary operator handling.The specific handling of the
Notoperator for boolean types in theEmitUnarymethod is a logical and necessary improvement.
286-289: Enhanced handling of equality operators.The specific handling of
NotEqualandEqualoperators for boolean types in theEmitBinaryOperatormethod is well-implemented and enhances clarity.runtime/ishtar.vm/VirtualMachine.cs (1)
930-930: Ensure correct handling ofTYPE_BOOLEANin jump conditions.The addition of
TYPE_BOOLEANhandling in the jump conditions needs careful verification to ensure it aligns with the expected logical flow and does not introduce any unintended behaviors. Consider adding unit tests specifically targeting these conditions to validate their correctness under various scenarios.Also applies to: 1131-1131
Verification successful
Ensure correct handling of
TYPE_BOOLEANin jump conditions.The provided test cases in
test/vc_test/expression_test.csconfirm thatTYPE_BOOLEANis being tested. Specifically, the tests validate that the result's type is correctly determined asTYPE_BOOLEAN.
test/vc_test/expression_test.cs
Assert.AreEqual(VeinTypeCode.TYPE_BOOLEAN, type.TypeCode);These tests should be sufficient to verify the correct handling of
TYPE_BOOLEANin jump conditions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of `TYPE_BOOLEAN` in jump conditions. # Test: Check for unit tests covering the new jump conditions with `TYPE_BOOLEAN`. rg --type cs $'TYPE_BOOLEAN' --files-with-matches | xargs rg 'test'Length of output: 182
Script:
#!/bin/bash # Description: Inspect the contents of `test/vc_test/expression_test.cs` for test cases involving `TYPE_BOOLEAN`. # Display the lines containing `TYPE_BOOLEAN` and surrounding context in the test file. rg --type cs --context 5 'TYPE_BOOLEAN' test/vc_test/expression_test.csLength of output: 548
| case EQL_T: | ||
| case EQL_F: | ||
| { | ||
| ++ip; | ||
| --sp; | ||
| var first = *sp; | ||
| *sp = _comparer(first, default, invocation->last_ip, invocation); | ||
| println($"$$$ {invocation->last_ip} : {debug_comparer_get_symbol(first, default, invocation->last_ip)} == {sp->data.i == 1}"); | ||
| sp++; | ||
| } | ||
| break; |
There was a problem hiding this comment.
Refine the new comparison operations for clarity and performance.
The new comparison operations introduced in these lines could benefit from a refactoring for clarity. Consider encapsulating the comparison logic into a separate method to improve readability and maintainability. Additionally, ensure that the operations are optimized for performance, given their critical role in the execution flow.
Also applies to: 822-836
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private stackval _comparer(in stackval first, in stackval second, OpCodeValue operation, CallFrame* frame) | ||
| { | ||
| if (operation is not EQL_F and not EQL_T && first.type != second.type) | ||
| { | ||
| switch (first.type) | ||
| { | ||
| case TYPE_I4 when second.type is TYPE_BOOLEAN: | ||
| break; | ||
| case TYPE_BOOLEAN when second.type is TYPE_I4: | ||
| break; | ||
| default: | ||
| FastFail(WNE.TYPE_MISMATCH, "", frame); | ||
| return default; | ||
| } | ||
| } | ||
|
|
||
| var result = (first.type) switch | ||
| { | ||
| TYPE_I4 or TYPE_BOOLEAN or TYPE_CHAR | ||
| => comparer(first.data.i, second.data.i, operation), | ||
| TYPE_I1 => comparer(first.data.b, second.data.b, operation), | ||
| TYPE_U1 => comparer(first.data.ub, second.data.ub, operation), | ||
| TYPE_I2 => comparer(first.data.s, second.data.s, operation), | ||
| TYPE_U2 => comparer(first.data.us, second.data.us, operation), | ||
| TYPE_U4 => comparer(first.data.ui, second.data.ui, operation), | ||
| TYPE_I8 => comparer(first.data.l, second.data.l, operation), | ||
| TYPE_U8 => comparer(first.data.ul, second.data.ul, operation), | ||
| TYPE_R2 => comparer(first.data.hf, second.data.hf, operation), | ||
| TYPE_R4 => comparer(first.data.f_r4, second.data.f_r4, operation), | ||
| TYPE_R8 => comparer(first.data.f, second.data.f, operation), | ||
| TYPE_R16 => comparer(first.data.d, second.data.d, operation), | ||
| TYPE_RAW => comparer(first.data.p, second.data.p, operation), | ||
| _ => throw new ArgumentOutOfRangeException() | ||
| }; | ||
|
|
||
| if (result is -1) | ||
| { | ||
| FastFail(WNE.STATE_CORRUPT, "", frame); | ||
| return default; | ||
| } | ||
|
|
||
| return new stackval() | ||
| { | ||
| data = { i = result }, | ||
| type = TYPE_BOOLEAN | ||
| }; | ||
| } |
There was a problem hiding this comment.
Refactor the _comparer method to simplify the switch-case logic.
Consider using a dictionary or a more dynamic dispatch mechanism to handle type comparisons, which could reduce the complexity and improve maintainability of this method.
Tools
GitHub Check: Codacy Static Code Analysis
[notice] 59-59: runtime/ishtar.vm/vm.shit.cs#L59
Remove these redundant parentheses.
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private string debug_comparer_get_symbol(in stackval first, in stackval second, OpCodeValue operation) | ||
| { | ||
| return (first.type) switch | ||
| { | ||
| TYPE_I4 or TYPE_BOOLEAN or TYPE_CHAR | ||
| => debug_comparer_get_symbol(first.data.i, second.data.i, operation), | ||
| TYPE_I1 => debug_comparer_get_symbol(first.data.b, second.data.b, operation), | ||
| TYPE_U1 => debug_comparer_get_symbol(first.data.ub, second.data.ub, operation), | ||
| TYPE_I2 => debug_comparer_get_symbol(first.data.s, second.data.s, operation), | ||
| TYPE_U2 => debug_comparer_get_symbol(first.data.us, second.data.us, operation), | ||
| TYPE_U4 => debug_comparer_get_symbol(first.data.ui, second.data.ui, operation), | ||
| TYPE_I8 => debug_comparer_get_symbol(first.data.l, second.data.l, operation), | ||
| TYPE_U8 => debug_comparer_get_symbol(first.data.ul, second.data.ul, operation), | ||
| TYPE_R2 => debug_comparer_get_symbol(first.data.hf, second.data.hf, operation), | ||
| TYPE_R4 => debug_comparer_get_symbol(first.data.f_r4, second.data.f_r4, operation), | ||
| TYPE_R8 => debug_comparer_get_symbol(first.data.f, second.data.f, operation), | ||
| TYPE_R16 => debug_comparer_get_symbol(first.data.d, second.data.d, operation), | ||
| TYPE_RAW => debug_comparer_get_symbol(first.data.p, second.data.p, operation), | ||
| _ => throw new ArgumentOutOfRangeException() | ||
| }; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private static string debug_comparer_get_symbol<TNumber>(TNumber first, TNumber second, OpCodeValue operation) | ||
| where TNumber : INumber<TNumber> | ||
| => (operation) switch | ||
| { | ||
| EQL_F => $"{first} == {TNumber.Zero}", | ||
| EQL_H => $"{first} > {second}", | ||
| EQL_L => $"{first} < {second}", | ||
| EQL_T => $"{first} == {TNumber.One}", | ||
| EQL_NN => $"{first} != {second}", | ||
| EQL_HQ => $"{first} >= {second}", | ||
| EQL_LQ => $"{first} <= {second}", | ||
| EQL_NQ => $"{first} == {second}", | ||
| _ => "INVALID" | ||
| }; |
There was a problem hiding this comment.
Enhance readability of the debug_comparer_get_symbol method.
This method could benefit from using a dictionary or a strategy pattern for operation handling to make the code more readable and extensible.
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private static int comparer<TNumber>( | ||
| TNumber first, | ||
| TNumber second, | ||
| OpCodeValue operation) | ||
| where TNumber : INumber<TNumber> => | ||
| (operation) switch | ||
| { | ||
|
|
||
| EQL_F when first == TNumber.Zero => _true, | ||
| EQL_F => _false, | ||
|
|
||
| EQL_H when first > second => _true, | ||
| EQL_H => _false, | ||
|
|
||
| EQL_L when first < second => _true, | ||
| EQL_L => _false, | ||
|
|
||
| EQL_T when first == TNumber.One => _true, | ||
| EQL_T => _false, | ||
|
|
||
| EQL_NN when first != second => _true, | ||
| EQL_NN => _false, | ||
|
|
||
| EQL_HQ when first >= second => _true, | ||
| EQL_HQ => _false, | ||
|
|
||
| EQL_LQ when first <= second => _true, | ||
| EQL_LQ => _false, | ||
|
|
||
| EQL_NQ when first == second => _true, | ||
| EQL_NQ => _false, | ||
|
|
||
| _ => -1 |
There was a problem hiding this comment.
Optimize the comparer method to handle edge cases more gracefully.
Consider adding more comprehensive handling for edge cases and potentially using a strategy pattern or similar to handle different types of comparisons.
| return new stackval() | ||
| { | ||
| data = { i = result }, | ||
| type = TYPE_BOOLEAN | ||
| }; |
There was a problem hiding this comment.
Remove redundant parentheses as suggested by static analysis.
- return new stackval()
+ return new stackvalCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new stackval() | |
| { | |
| data = { i = result }, | |
| type = TYPE_BOOLEAN | |
| }; | |
| return new stackval | |
| { | |
| data = { i = result }, | |
| type = TYPE_BOOLEAN | |
| }; |
Tools
GitHub Check: Codacy Static Code Analysis
[notice] 59-59: runtime/ishtar.vm/vm.shit.cs#L59
Remove these redundant parentheses.
| try | ||
| { | ||
| return ExitScope(); | ||
| } | ||
| catch (CannotExistMainScopeException e) | ||
| { | ||
| if (!allowExitFromRoot) | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Consider simplifying exception handling.
The exception handling in ExitScope could be simplified by removing the unnecessary try-catch block if the logic does not specifically require it.
- try
- {
- return ExitScope();
- }
- catch (CannotExistMainScopeException e)
- {
- if (!allowExitFromRoot)
- throw;
- }
+ if (!allowExitFromRoot)
+ throw new CannotExistMainScopeException();
+ return ExitScope();Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try | |
| { | |
| return ExitScope(); | |
| } | |
| catch (CannotExistMainScopeException e) | |
| { | |
| if (!allowExitFromRoot) | |
| throw; | |
| } | |
| if (!allowExitFromRoot) | |
| throw new CannotExistMainScopeException(); | |
| return ExitScope(); |
| @@ -22,15 +22,32 @@ | |||
| } | |||
| } | |||
|
|
|||
| private static (string line, string arrow_line) NewDiffError(Transform t, DocumentDeclaration doc) | |||
| public static string DiffErrorFull(this Transform t, FileInfo doc) | |||
There was a problem hiding this comment.
Add missing import for FileInfo.
The FileInfo class is used but not imported, causing a build failure. Add the necessary using System.IO; directive at the top of the file.
+ using System.IO;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static string DiffErrorFull(this Transform t, FileInfo doc) | |
| + using System.IO; | |
| public static string DiffErrorFull(this Transform t, FileInfo doc) |
Tools
GitHub Check: build_all (ubuntu-latest, linux-x64, true)
[failure] 25-25:
The type or namespace name 'FileInfo' could not be found (are you missing a using directive or an assembly reference?)
| if (!ctx.DisableOptimization && ifStatement.Expression is BoolLiteralExpressionSyntax @bool) | ||
| { | ||
| if (@bool.Value) | ||
| { | ||
| generator.EmitStatement(ifStatement.ThenStatement); | ||
| if (ifStatement.ElseStatement is not null) | ||
| generator.Emit(OpCodes.JMP, endLabel); | ||
| } |
There was a problem hiding this comment.
Optimize conditional branches.
The branching logic in EmitIfElse can be optimized by consolidating checks and reducing duplicate code.
- if (!ctx.DisableOptimization && ifStatement.Expression is BoolLiteralExpressionSyntax @bool)
- {
- if (@bool.Value)
- {
- generator.EmitStatement(ifStatement.ThenStatement);
- if (ifStatement.ElseStatement is not null)
- generator.Emit(OpCodes.JMP, endLabel);
- }
- else
- generator.Emit(OpCodes.JMP, elseLabel);
- }
+ if (!ctx.DisableOptimization && ifStatement.Expression is BoolLiteralExpressionSyntax @bool && @bool.Value)
+ {
+ generator.EmitStatement(ifStatement.ThenStatement);
+ if (ifStatement.ElseStatement is not null)
+ generator.Emit(OpCodes.JMP, endLabel);
+ }
+ else if (!ctx.DisableOptimization && ifStatement.Expression is BoolLiteralExpressionSyntax @bool && [email protected])
+ generator.Emit(OpCodes.JMP, elseLabel);Also applies to: 31-34
|
|
||
| public static void Error(string text) => _print(text, null, null, errors); | ||
| public static void Error(string text, BaseSyntax posed) => _print(text, posed, null, errors); | ||
| public static void Error(string text, BaseSyntax posed, DocumentDeclaration doc) | ||
| => _print(text, posed, doc.FileEntity, errors); | ||
| public static void Error(string text, BaseSyntax posed, FileInfo doc) |
There was a problem hiding this comment.
Ensure consistent error handling.
The method _print is used in different ways, which could lead to inconsistencies in how logs are handled. Consider standardizing the input parameters and the way errors are logged.
- public static void Error(string text, BaseSyntax posed, FileInfo doc)
- => _print(text, posed, doc, errors);
+ public static void Error(string text, BaseSyntax posed, FileInfo doc)
+ {
+ if (doc == null) throw new ArgumentNullException(nameof(doc));
+ _print(text, posed, doc, errors);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static void Error(string text, BaseSyntax posed, FileInfo doc) | |
| public static void Error(string text, BaseSyntax posed, FileInfo doc) | |
| { | |
| if (doc == null) throw new ArgumentNullException(nameof(doc)); | |
| _print(text, posed, doc, errors); | |
| } |
| if (exp is UnaryExpressionSyntax unary) | ||
| { | ||
| if (unary.OperatorType.IsLogic()) | ||
| return VeinTypeCode.TYPE_BOOLEAN.AsClass()(Types.Storage); | ||
| // todo | ||
| } |
There was a problem hiding this comment.
Incomplete implementation of unary operator handling.
Please complete the implementation for unary operators as indicated by the "todo" comment.
Summary by CodeRabbit
New Features
DisableOptimizationfor compile commands.Improvements
Bug Fixes