Conversation
Also eliminate the generic TC_ERR_OTHER which is not needed.
The dead code can be stripped later! This way there is no need to include anything, all the things are already there. TODO: Add some way to add stdlib by choice, maybe '-stdlib' ?
All of stdlib/ should now compile!
Adding with flag means stdlib/ should be compiled aswell. The examples test script now look for a file '?.stdlib' to determine if the example needs to have stdlib/ compiled in. All examples which do not need it can compile faster!
WalkthroughThe changes update several function signatures to return booleans or signed integers to signal errors, introduce more specific error codes in the type checker, and add additional error checking and handling. The CLI flag system is enhanced with a new "stdlib" flag and updated flag processing logic. Additionally, some standard library files have been added or modified, including new string and assert functions, while multiple test and collection files have been revised or removed. Changes
Sequence Diagram(s)sequenceDiagram
participant E as emit_asm_avr_basic_block
participant AR as allocate_registers
participant ART as allocate_registers_single_tac
E->>AR: Call allocate_registers()
AR->>ART: For each TAC, call allocate_registers_single_tac()
ART-->>AR: Return success status (bool)
AR-->>E: Aggregate and return status (bool)
E->>E: If status is false, print error and exit
sequenceDiagram
participant M as main
participant MF as makeFlags
participant FS as flags_stdlib
M->>MF: Call makeFlags(argc, argv)
MF-->>M: Return Flags structure
Note right of M: Check if "stdlib" flag is set using flags_stdlib
M->>M: Configure compilation based on stdlib flag
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for espl1000 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
compiler/main/typechecker/tcctx.h (1)
39-39: Improved naming for dereferencing errors.Replacing the generic
TC_ERR_OTHERwith the specificTC_ERR_CANNOT_DEREF_THISenhances error reporting clarity. This change aligns with the updates in tc_address_of.c and tc_deref.c.Consider adding small comments for these error types (similar to the "unary operator" comment at line 29) to explain their purpose.
TC_ERR_INDEX_NOT_INTEGER_TYPE, TC_ERR_TOO_MANY_INDICES, +//cannot use indices on non-local variables TC_ERR_CANNOT_INDEX_INTO, TC_ERR_LOCAL_VAR_NOT_A_SUBROUTINE, TC_ERR_SIDE_EFFECT_IN_PURE_CONTEXT, +//cannot dereference this type TC_ERR_CANNOT_DEREF_THIS,compiler/main/typechecker/util/tc_errors.c (1)
9-40: Consider adding comments for each error type.While the error messages are self-explanatory to a degree, adding comments explaining when each error type is triggered and providing examples would improve maintainability.
stdlib/collections/set.dg (1)
26-29: Return Value Semantics inset_removeThe function currently returns
trueregardless of whether the element was present or successfully removed. If intended to signal the removal status (e.g., whether an element was actually removed), you might consider returningfalsewhen the element is not found. Please confirm that this behavior matches the overall design of your standard library.stdlib/avr/portb.dg (1)
12-17: Inconsistent variable naming inportb_writefunction.While you've renamed variables in the other functions to use
ptr, theportb_writefunction still usesportb_data. Consider renaming this variable toptras well for consistency across all functions in this file.fn portb_write(uint8 value) -> int { - *uint8 portb_data = 0x25; - *portb_data = value; + *uint8 ptr = 0x25; + *ptr = value; return 0; }stdlib/base/assert.dg (1)
1-10: Simple assert function implementation looks goodThis assert function follows the standard pattern for assertions - checking a condition and aborting execution if it fails. The TODO comment indicates future plans to enhance it with error message printing.
Consider enhancing the assert function to accept an error message parameter when implementing the TODO. This would make assertions more informative:
-fn assert(bool x) -> int { +fn assert(bool x, string message) -> int { //TODO: print error message if (!x){ + // print message here exit(1); } return 0; }compiler/main/cli/flags/all_flags.c (1)
106-112: New stdlib flag looks goodThe new flag for enabling standard library compilation is properly implemented following the existing pattern.
Minor typo in the description - "aswell" should be "as well".
-.description = "compile stdlib/ aswell", +.description = "compile stdlib/ as well",stdlib/avr/usart.dg (3)
6-8: Unify pointer type and consider usingvolatilefor memory-mapped registers.
Within embedded systems, hardware registers typically require avolatilepointer declaration to avoid compiler optimizations that skip memory reads/writes. Also note that this function (usart_tx_char) uses*char ptr = 0xc6;while the next function uses auint8pointer. Consider aligning pointer types to prevent confusion and ensure consistent data handling.
16-17: Maintain consistent pointer usage across USART transmit functions.
Theusart_txfunction uses*uint8 ptr = 0xc6;, contrasting with the priorusart_tx_charfunction, which uses*char ptr = 0xc6;. For clarity and consistency, unify the pointer type (e.g.,uint8orchar), and if appropriate for hardware registers, addvolatile.
74-75: Consider checking the return values ofusart_init_baudrate_ubbr0landusart_init_baudrate_ubbr0h.
Although these functions currently always return 0 on success, explicitly verifying their return codes (especially if you extend error handling later) can make debugging more robust.stdlib/base/string.dg (1)
3-11: Validatestrlenfor null-terminated strings and edge cases.
This function correctly increments through characters until'\0'. However, consider handling a potential null pointer or verifying the string is indeed null-terminated to prevent undefined behavior ifstrpoints to an invalid memory region.compiler/main/cli/flags/flags.c (1)
340-344: Consider printing an error message ifflags_add_stdlibfails.
Currently,makeFlagsreturnsNULLon failure without explaining why. A brief error message (like “Failed to add stdlib files.”) would help users diagnose the issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
compiler/main/avr_code_gen/cg_avr_basic_block.c(5 hunks)compiler/main/avr_code_gen/cg_avr_single_function.c(1 hunks)compiler/main/cli/flags/all_flags.c(1 hunks)compiler/main/cli/flags/flags.c(6 hunks)compiler/main/cli/flags/flags.h(2 hunks)compiler/main/cli/main.c(1 hunks)compiler/main/typechecker/tc_address_of.c(2 hunks)compiler/main/typechecker/tc_deref.c(1 hunks)compiler/main/typechecker/tc_var.c(1 hunks)compiler/main/typechecker/tcctx.h(1 hunks)compiler/main/typechecker/util/tc_errors.c(5 hunks)compiler/test/gen_tac/util/test_gen_tac_util.c(1 hunks)compiler/test/typechecker/test-src/all_errors.dg(1 hunks)compiler/test/x86_code_gen/test_x86_code_gen_util.c(1 hunks)examples/assert_examples.sh(2 hunks)examples/led_blink_timer/Makefile(1 hunks)rat/rat.c(2 hunks)rat/rat.h(1 hunks)stdlib/avr/portb.dg(2 hunks)stdlib/avr/usart.dg(5 hunks)stdlib/base/assert.dg(1 hunks)stdlib/base/math.dg(1 hunks)stdlib/base/primes.dg(2 hunks)stdlib/base/string.dg(1 hunks)stdlib/collections/arraylist.dg(1 hunks)stdlib/collections/intbintree.dg(0 hunks)stdlib/collections/linkedlist.dg(1 hunks)stdlib/collections/set.dg(1 hunks)stdlib/test.dg(0 hunks)stdlib/tests/collections/test_array.dg(0 hunks)stdlib/tests/collections/test_arraylist.dg(0 hunks)stdlib/tests/collections/test_intbintree.dg(0 hunks)stdlib/tests/collections/test_linkedlist.dg(0 hunks)stdlib/tests/collections/test_set.dg(0 hunks)stdlib/tests/collections/test_stack.dg(0 hunks)stdlib/tests/test_math.dg(1 hunks)stdlib/tests/test_polynomial.dg(0 hunks)
💤 Files with no reviewable changes (9)
- stdlib/test.dg
- stdlib/tests/collections/test_intbintree.dg
- stdlib/tests/test_polynomial.dg
- stdlib/tests/collections/test_stack.dg
- stdlib/tests/collections/test_linkedlist.dg
- stdlib/tests/collections/test_array.dg
- stdlib/tests/collections/test_arraylist.dg
- stdlib/tests/collections/test_set.dg
- stdlib/collections/intbintree.dg
🧰 Additional context used
🧬 Code Definitions (7)
compiler/main/typechecker/tc_address_of.c (1)
compiler/main/typechecker/util/tc_errors.c (1) (1)
error_snippet_and_msg(93-113)
compiler/test/gen_tac/util/test_gen_tac_util.c (2)
compiler/main/cli/flags/flags.h (1) (1)
makeFlags(11-11)compiler/main/cli/flags/flags.c (1) (1)
makeFlags(309-360)
compiler/test/x86_code_gen/test_x86_code_gen_util.c (2)
compiler/main/cli/flags/flags.h (1) (1)
makeFlags(11-11)compiler/main/cli/flags/flags.c (1) (1)
makeFlags(309-360)
compiler/main/cli/main.c (2)
compiler/main/cli/flags/flags.h (1) (1)
makeFlags(11-11)compiler/main/cli/flags/flags.c (1) (1)
makeFlags(309-360)
compiler/main/typechecker/tc_var.c (1)
compiler/main/typechecker/util/tc_errors.c (1) (1)
error(83-86)
compiler/main/typechecker/tc_deref.c (1)
compiler/main/typechecker/util/tc_errors.c (1) (1)
error_snippet_and_msg(93-113)
compiler/main/avr_code_gen/cg_avr_basic_block.c (10)
tac/tac.h (1) (1)
tac_dest(99-99)compiler/main/util/ctx.c (1) (1)
ctx_flags(62-64)compiler/main/util/ctx.h (1) (1)
ctx_flags(16-16)compiler/main/cli/flags/flags.c (1) (1)
flags_debug(396-398)compiler/main/cli/flags/flags.h (1) (1)
flags_debug(23-23)rat/rat.c (2) (2)
rat_ensure_register(230-251)rat_is_wide(253-260)rat/rat.h (2) (2)
rat_ensure_register(59-59)rat_is_wide(61-61)tac/tacbuffer.c (1) (1)
tacbuffer_count(116-118)tac/tacbuffer.h (1) (1)
tacbuffer_count(32-32)tac/tac.c (1) (1)
tac_dest(46-72)
🪛 Shellcheck (0.10.0)
examples/assert_examples.sh
[warning] 22-22: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 58-58: In POSIX sh, [[ ]] is undefined.
(SC3010)
🔇 Additional comments (48)
stdlib/collections/linkedlist.dg (1)
38-38: Variable renaming looks goodThe variable
indexhas been consistently renamed toithroughout thelinkedlist_index_offunction. This change improves naming consistency across the codebase.Also applies to: 43-43, 47-47, 51-51
stdlib/collections/arraylist.dg (1)
15-15: Parameter renaming looks goodThe parameter
indexhas been consistently renamed toiin both the function signature and its usage. This change aligns with similar renaming in the linkedlist implementation, creating better consistency across collection utilities.Also applies to: 17-17
compiler/main/typechecker/tc_var.c (1)
55-55: Improved error specificity with new error code.The change from using a generic error code to the more specific
TC_ERR_CANNOT_INDEX_INTOimproves error reporting by providing clearer categorization when attempting to index into non-local variables. This makes debugging easier for compiler users.compiler/main/typechecker/tc_address_of.c (2)
30-30: Enhanced error reporting specificity.Replacing the generic error code with
TC_ERR_CANNOT_DEREF_THISprovides more precise error categorization when attempting to dereference a subroutine type. This change makes error diagnosis more straightforward.
60-60: Improved error specificity with consistent error code.Using the specific
TC_ERR_CANNOT_DEREF_THISerror code here maintains consistency with the earlier change in this file and aligns with the overall effort to improve error reporting granularity throughout the type checker.compiler/main/typechecker/tcctx.h (1)
34-34: Well-placed new error type for indexing errors.The addition of
TC_ERR_CANNOT_INDEX_INTOis appropriately placed after other index-related errors, providing a specific error code for when indexing is attempted on non-local variables.compiler/main/typechecker/tc_deref.c (1)
32-32: Consistent error handling improvement.The change from a generic error code to
TC_ERR_CANNOT_DEREF_THISprovides more precise error categorization when attempting to dereference a type that's neither a pointer nor an array. This change is consistent with similar improvements in tc_address_of.c.compiler/main/typechecker/util/tc_errors.c (6)
33-33: Added a new specific error message for indexing operations.The addition of the
TC_ERR_CANNOT_INDEX_INTOerror message improves error reporting by providing a more specific message when users attempt to index into a non-indexable type. This is a good addition that aligns with the existing error message structure.
39-39: Replaced generic error with more specific dereferencing error.Changing
TC_ERR_OTHERtoTC_ERR_CANNOT_DEREF_THISimproves the specificity of error messages, making it clearer to users what went wrong when they try to dereference something that cannot be dereferenced.
52-63: Improved constructor signature by directly accepting error kind parameter.The updated constructor now receives the error kind as a parameter instead of setting it later, which reduces the chance of errors and makes the code more maintainable.
68-81: Refactored error insertion logic with better context handling.The replacement of
obtain_next_errorwithtc_insert_errorimproves the error management by:
- Taking the typechecker context as a parameter
- Properly handling the case when no errors exist yet
- Maintaining the linked list of errors correctly
This change appears to be part of a larger effort to improve error handling throughout the codebase.
95-99: Added null check for error creation.Good addition of the null check after creating a new error. This prevents potential segmentation faults or undefined behavior if memory allocation fails.
108-108: Updated to use the new error insertion function.Consistent update to use the new
tc_insert_errorfunction instead of the previous approach.stdlib/collections/set.dg (2)
31-31: Enhanced Variable Naming withidxRenaming the variable to
idximproves clarity and consistency across the function.
33-36: Accurate Index Assignment for Element LookupThe assignment of the found index to
idxis implemented correctly. This update maintains consistency with the new variable name, ensuring that the correct element position is captured for subsequent removal.stdlib/base/math.dg (1)
30-30: Code simplification looks good.The change from using the shorthand operator (
res *= k) to an explicit multiplication assignment (res = res * k) maintains the same functionality while potentially improving clarity. This change doesn't affect the correctness of the factorial calculation.stdlib/tests/test_math.dg (2)
1-1: Function renaming fromtest_mathtotestmathlooks appropriate.The removal of the underscore in the function name helps maintain consistent naming conventions. This change aligns with standard coding practices.
6-6: Good update to useipowinstead ofpow.The assertion has been correctly updated to test
ipow(2,3)instead ofpow(2,3), aligning with the function declared in math.dg. This ensures that the proper integer power function is being tested.stdlib/base/primes.dg (2)
2-2: Comment formatting fix looks good.The whitespace in the comment has been corrected, improving formatting consistency.
11-11: Improved expression for modulus calculation.Changing from
mod(n, k)to the more directn % kis appropriate, as it uses the built-in modulo operator instead of a function call. This likely improves both readability and performance.stdlib/avr/portb.dg (2)
7-8: Variable naming standardization looks good.Renaming
portb_ddrdto the more genericptris appropriate since it's just a local pointer variable. The functionality remains unchanged while improving code consistency.
21-22: Variable naming standardization looks good.Similarly, renaming
portb_datatoptrin theportb_readfunction maintains consistent naming conventions for local pointer variables. The functionality remains unchanged.compiler/test/typechecker/test-src/all_errors.dg (1)
39-39: Minor whitespace changeThis is just a whitespace change that doesn't affect the functionality of this test file.
compiler/main/avr_code_gen/cg_avr_single_function.c (1)
50-52: Good error handling improvementThe added error handling for register allocation failures is a good improvement that provides clear feedback when something goes wrong.
examples/led_blink_timer/Makefile (1)
4-4: Simplified build process using the new-stdlibflagThe compile target has been updated to use the
-stdlibflag instead of explicitly listing standard library files. This change simplifies the Makefile and makes it more maintainable.compiler/test/gen_tac/util/test_gen_tac_util.c (1)
32-32: Updated function call to match new signatureThe call to
makeFlags()has been updated to match its new signature which no longer takes a boolean parameter. This aligns with the changes in the flags system where the stdlib handling is now determined by the presence of the-stdlibflag rather than a function parameter.examples/assert_examples.sh (2)
20-24: Added conditional stdlib flag supportThe script now checks for the existence of a
.stdlibfile to determine whether to include the-stdlibflag when compiling. This provides more flexibility in testing examples with or without the standard library.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 22-22: In POSIX sh, [[ ]] is undefined.
(SC3010)
56-61: Duplicated conditional stdlib flag support for stdout testsThe same conditional stdlib flag logic has been correctly implemented for the stdout testing section, ensuring consistent behavior across both test types.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 58-58: In POSIX sh, [[ ]] is undefined.
(SC3010)
rat/rat.h (1)
58-59: Improved error handling with negative return valuesThe return type of
rat_ensure_registerhas been changed fromuint32_ttoint32_tto allow for negative values to indicate errors. This change makes the function more consistent with other error-reporting functions in the file and provides better error handling.compiler/main/cli/main.c (1)
33-33: Function call updated to match the new signatureThe
makeFlagscall has been updated to remove the third parameter, aligning with the new function signature inflags.h. This change is part of a refactoring that moves stdlib inclusion logic into themakeFlagsfunction, controlled by the new stdlib flag.rat/rat.c (2)
230-230: Return type changed to support error signalingThe function signature has been updated to return
int32_tinstead ofuint32_t, allowing the function to return negative values to indicate errors. This change improves error handling by enabling callers to detect when register allocation fails.
241-245: Added error checking and propagationThe variable
regis now declared as anintto match the return type ofrat_get_free_register, and a check has been added to return -1 if no register is available. This properly propagates errors up the call stack, enhancing the robustness of the register allocation system.compiler/test/x86_code_gen/test_x86_code_gen_util.c (1)
427-427: Function call updated to match the new signatureThe
makeFlagscall has been updated to remove the third parameter, aligning with the new function signature inflags.h. This ensures consistency with the architectural changes that move stdlib inclusion logic into the flags system.compiler/main/cli/flags/flags.h (2)
11-11: Function signature simplifiedThe
makeFlagsfunction signature has been updated to remove thebool add_stdlibparameter, simplifying the interface. This change improves modularity by moving the stdlib inclusion logic inside the function implementation.
35-35: Added new function for stdlib flag checkingA new function
flags_stdlibhas been added to check if the stdlib flag is set. This function enhances the modularity of the code by encapsulating the logic for checking the stdlib flag, supporting the refactored approach to standard library inclusion.stdlib/avr/usart.dg (3)
51-57: Segmented baud-rate initialization appears clear.
This new function writes the low byte of the baud-rate register. The approach is straightforward and helps with code maintainability by splitting the logic into smaller functions.
86-87: Verify intended bits for UCSR0B.
Right now,*ptr = 0b01000;sets only TX Enable (bit 3). Typically, enabling both receive (bit 4) and transmit (bit 3) requires0b11000. Double-check this matches your hardware requirements.
98-98: Confirm the bit pattern for UCSR0C.
0b00101110sets multiple bits, including 2 stop bits and 7-bit data? Ensure these bits match the desired frame format from the device datasheet (e.g., 8 data bits, parity off, 2 stop bits).stdlib/base/string.dg (1)
57-70: Verify recursion with numeric digit vs ASCII checks inprint_uint64.
The logic for splitting off the last digit and recursively printing the higher-order digits is correct in principle. However, it passesdtoprint_digit, which checks against'0'...'9'. Updateprint_digitto handle numeric digits or convert them to ASCII properly.compiler/main/avr_code_gen/cg_avr_basic_block.c (5)
18-22: Documenting return types for register allocation is helpful.
Clearly stating “false on error” in the comments clarifies the function guarantees and aids maintainers.
46-46: Good practice: store and check the returned boolean status.
Capturingallocate_registers(...)instatusensures subsequent error handling. This pattern is essential for preventing silent failures in code generation.
52-55: Helpful error message for register allocation failure.
Printing an error before exiting the function clarifies the cause of the compilation error, improving diagnostics.
88-99:allocate_registersfunction properly checks each TAC.
Eachallocate_registers_single_taccall’s result is verified, returning false immediately on failure. This short-circuits further processing when an issue arises.
101-169: Returning a boolean fromallocate_registers_single_tacensures consistent error handling.
Switching fromvoidtoboolto capturerat_ensure_registerfailures is a valuable enhancement. The direct returns (e.g.,return rat_ensure_register(...) > 0;) keep each TAC case concise. Also, the default “return true” ensures unrecognized TACs don’t trigger unintended errors.compiler/main/cli/flags/flags.c (4)
9-11: Including necessary headers for directory and file operations.
These#includestatements are required to handle directory traversal, file status checks, and related I/O functionality.
236-301: Check for errors fromasprintfand additional directory handling corner cases.
The function proceeds tostat(path, &path_stat)without verifyingasprintfsuccess. Consider verifyingpath != NULLto avoid unexpected behavior. Also ensure thatclosedir(dir)branches are consistent if partial recursion fails.
303-307: Encapsulating stdlib addition logic in a dedicated function is clear.
flags_add_stdlibkeeps the overall code simpler and generic. Just confirm that failing to add the stdlib logs an error or otherwise informs the user.
433-435:flags_stdlibcleanly checks if thestdlibflag is set.
This concise wrapper aligns with otherflags_*boolean helpers (e.g.,flags_debug,flags_help).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
examples/stdlib/base/test_string.dg (1)
2-7: Function looks good, but add a descriptive comment.The
mainfunction correctly tests theprintln_uint64function with an expected value and returns 0 to indicate success. Consider adding a brief comment explaining the purpose of this test file to improve maintainability.+// Test the println_uint64 function from the string stdlib module fn main () ~> int { println_uint64(8329); return 0; }stdlib/base/string.dg (3)
2-11: Remove "untested" comment and improve error handling.The
strlenfunction implementation is correct, but consider these improvements:
- Remove the "untested" comment now that you have a test file
- Consider adding boundary checks for null pointers
-// untested fn strlen(*char str) -> uint64 { + if str == null { + return 0; + } uint64 l = 0; while *str != '\0' { l++; str += 1; } return l; }
72-79: Simplify println implementation.The current implementation creates unnecessary variables. You can directly write the newline character to stdout.
fn println() -> int { - [char] str1 = "\n"; - *char ptr = str1; - write(1, ptr, 1); + write(1, "\n", 1); return 0; }
81-87: Add error handling to println_uint64.The function should check and propagate error codes from the functions it calls to ensure proper error handling.
fn println_uint64(uint64 x) -> int { - print_uint64(x); - println(); + int result = print_uint64(x); + if result != 0 { + return result; + } + + return println(); - return 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/stdlib/base/test_string.dg(1 hunks)examples/stdlib/base/test_string.stdout(1 hunks)stdlib/base/string.dg(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/stdlib/base/test_string.stdout
🔇 Additional comments (1)
stdlib/base/string.dg (1)
57-70: Check for error handling in print_uint64.The
print_uint64function should propagate the error code fromprint_digitif it fails. This ensures robust error handling throughout the call chain.Consider checking if any digits are being printed that are larger than 9, which would cause print_digit to fail in my refactored version.
fn print_uint64(uint64 x) -> int { uint64 d = x % 10; uint64 rest = x / 10; if rest != 0{ - print_uint64(rest); + int result = print_uint64(rest); + if result != 0 { + return result; + } } - print_digit(d); + return print_digit(d); - return 0; }
Function should be comparing the digit, not the ascii char.
pointbazaar
force-pushed
the
compile-stdlib
branch
from
a9da11a to
1243b54
Compare
pointbazaar
force-pushed
the
compile-stdlib
branch
from
589bea4 to
48909ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stdlib/avr/usart.dg (1)
99-100: Consider documenting the bit values in UCSR0C.The value
0b00101110is set without clear explanation of which bits correspond to which settings. Adding comments to explain what each bit represents would improve maintainability.*uint8 ptr = 0xc2; -*ptr = 0b00101110; +*ptr = 0b00101110; // USBS0 (bit 3) = 1 for 2 stop bits, UCSZ01:00 (bits 2:1) = 11 for 8 data bits
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stdlib/avr/usart.dg(5 hunks)
🔇 Additional comments (7)
stdlib/avr/usart.dg (7)
6-10: Implementation is more explicit with pointer dereferencing.The change from direct memory access with bracket notation to explicit pointer dereferencing looks good. This provides clearer intent about what's happening with the memory manipulation.
18-19: Consistent pointer usage looks good.The consistent application of pointer dereferencing throughout the file is a good improvement for code clarity and maintainability.
26-27: Memory access pattern is now consistent.The use of pointer dereferencing for reading the UCSR0A register maintains consistency with the rest of the changes in this file.
Also applies to: 33-33
53-59: Good modularization of the baud rate initialization.Breaking out the UBRR0L initialization into a separate function improves code organization and maintainability.
61-65: Add a return statement to match the function's declared return type.The function
usart_init_baudrate_ubbr0hhas a~> uint8signature but lacks areturnstatement.fn usart_init_baudrate_ubbr0h(uint16 value) ~> uint8 { *uint8 ptr = 0xc5; *ptr = (value >> 8) & 0xff; + return 0; }
76-77: Good use of extracted functions for baud rate initialization.Using the specialized functions for setting UBRR0L and UBRR0H improves code organization.
88-89: Verify the UCSR0B bit settings.The comment mentions enabling both receiver and transmitter (RXEN0 and TXEN0), but the value
0b01000only sets bit 4 (RXEN0).According to the comment, you should be setting both RXEN0 (bit 4) and TXEN0 (bit 3):
*uint8 ptr = 0xc1; -*ptr = 0b01000; +*ptr = 0b00011000; // Enable both RXEN0 (bit 4) and TXEN0 (bit 3)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stdlib/avr/usart.dg(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
stdlib/avr/usart.dg (1)
Learnt from: pointbazaar
PR: pointbazaar/espl1000#147
File: stdlib/avr/usart.dg:24-25
Timestamp: 2025-03-20T22:42:06.497Z
Learning: The ESPL compiler treats all memory reads as volatile by default, unlike C compilers which may optimize out repeated reads without the `volatile` keyword.
🔇 Additional comments (6)
stdlib/avr/usart.dg (6)
6-10: Clean refactoring to use pointer dereferencingThe change from direct memory access to pointer dereferencing is a good approach for maintaining consistency throughout the codebase. The return statement is properly included.
18-19: Consistent use of pointer dereferencing looks goodThe replacement of direct memory access with pointer dereferencing is consistently applied across the codebase, improving readability.
26-28: Pointer dereferencing implementation is correctThe implementation correctly uses pointer dereferencing to read the UCSR0A register status. As noted in previous discussions, the ESPL compiler treats all memory reads as volatile by default, so no explicit volatile keyword is needed.
Also applies to: 32-33
53-59: Well-structured new function with proper return valueThe newly added function
usart_init_baudrate_ubbr0lis well-structured, has a clear purpose of setting the low byte of the baud rate register, and correctly includes a return statement.
61-67: Well-structured function with fixed return statementThe function
usart_init_baudrate_ubbr0hcorrectly sets the high byte of the baud rate register and now includes the return statement that was missing in previous versions.
78-79: Good refactoring to use helper functionsThe refactoring to use the newly created helper functions improves code organization and maintainability.
Issues found in code review.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
compiler/main/cli/flags/flags.c (1)
325-331: 🛠️ Refactor suggestionPotential under-allocation of filenames array.
The
filenamesarray is allocated based onargc, but additional files may be added throughflags_add_stdliblater, potentially exceeding this initial allocation.Allocate a smaller initial capacity and let
flags_add_filenamehandle growth as needed:- flags->capacity_filenames = argc + 1; - flags->filenames = malloc(sizeof(char*) * argc); + // Start with a small capacity; flags_add_filename will grow as needed + flags->capacity_filenames = 8; // Or another reasonable starting value + flags->filenames = malloc(sizeof(char*) * flags->capacity_filenames);🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 329-329: Memory leak
(memleak)
🧹 Nitpick comments (2)
compiler/main/cli/flags/flags.c (2)
213-234: Optimize memory reallocation strategy.The current implementation directly sets
capacity_filenamestocount_filenames + 1, causing reallocation on every single call. This is inefficient for adding multiple filenames.Consider using a growth factor approach to reduce the number of reallocations:
- flags->capacity_filenames = flags->count_filenames + 1; + // Only reallocate if we've reached capacity + if (flags->count_filenames >= flags->capacity_filenames) { + // Use a growth factor (e.g., 1.5x or 2x) for better amortized performance + flags->capacity_filenames = flags->capacity_filenames == 0 ? 1 : flags->capacity_filenames * 2; + char** new_filenames = realloc(flags->filenames, flags->capacity_filenames * sizeof(char*)); + + if (!new_filenames) { + return false; + } + + flags->filenames = new_filenames; + }
239-304: Add safeguard against excessive recursion depth.The recursive function doesn't limit recursion depth, which could lead to stack overflow in deeply nested directory structures.
Consider adding a depth limit parameter:
-static bool flags_add_stdlib_recursive(struct Flags* flags, const char* base_path) { +static bool flags_add_stdlib_recursive(struct Flags* flags, const char* base_path, int depth) { + // Prevent excessive recursion + if (depth > 10) { // Choose appropriate max depth + fprintf(stderr, "Warning: Maximum directory depth reached at %s\n", base_path); + return true; + } DIR* dir = opendir(base_path); // ... (existing code) // When making recursive call: - if (!flags_add_stdlib_recursive(flags, path)) { + if (!flags_add_stdlib_recursive(flags, path, depth + 1)) {Then update the call in
flags_add_stdlib:char* base_path = (ESPL_STDLIB_PATH); - return flags_add_stdlib_recursive(flags, base_path); + return flags_add_stdlib_recursive(flags, base_path, 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
compiler/main/cli/flags/flags.c(6 hunks)
🔇 Additional comments (2)
compiler/main/cli/flags/flags.c (2)
343-347: LGTM - Proper stdlib integration.The implementation correctly checks the stdlib flag and handles errors appropriately when adding standard library files.
436-438: LGTM - Good encapsulation.Adding the
flags_stdlibfunction provides good encapsulation of the stdlib flag check, making the code more modular and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
compiler/main/cli/flags/flags.c (2)
286-294: Consider adding clarifying comments for directory skipping logic.The conditions for skipping directories are somewhat complex. Adding more detailed comments would make the code more maintainable, explaining why certain directories are included or excluded based on compilation targets.
if ((strcmp(name, "avr") == 0) && flags_x86(flags)) { - // do not include 'avr/' if we are compiling for x86 + // Skip AVR-specific directories when targeting x86 architecture continue; } if ((strcmp(name, "avr") != 0) && !flags_x86(flags)) { - // only include 'avr/' if we are compiling for avr + // When targeting AVR, only include AVR-specific directories and exclude generic ones continue; }
312-315: Consider error handling for non-existent stdlib path.The function assumes that
ESPL_STDLIB_PATHis a valid directory. Consider adding a check to verify that the path exists before attempting to open it.static bool flags_add_stdlib(struct Flags* flags) { char* base_path = (ESPL_STDLIB_PATH); + + // Verify that the stdlib path exists + struct stat st; + if (stat(base_path, &st) != 0 || !S_ISDIR(st.st_mode)) { + fprintf(stderr, "Error: Standard library path '%s' does not exist or is not a directory\n", base_path); + return false; + } + return flags_add_stdlib_recursive(flags, base_path); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.circleci/config.yml(1 hunks)compiler/main/cli/flags/flags.c(6 hunks)
🔇 Additional comments (7)
.circleci/config.yml (2)
134-145: Great addition of shellcheck for shell script validation!Adding a dedicated job to validate shell scripts with shellcheck is an excellent improvement to code quality. This will catch potential issues in shell scripts early in the CI pipeline.
153-153: LGTM: Proper integration into the workflow.The new job is correctly integrated into the build workflow, ensuring it runs alongside other validation steps.
compiler/main/cli/flags/flags.c (5)
9-11: Appropriate includes for directory operations.The added header files are necessary for the directory traversal functionality implemented in this file.
213-234: Well-implemented memory-safe filename addition function.Good implementation of the
flags_add_filenamefunction with proper memory allocation error handling. The use of a temporary variable for realloc prevents potential memory leaks.
348-352: Good integration of stdlib flag handling.The code properly checks if the stdlib flag is set and handles potential errors from adding standard library files.
441-443: Good modular design with dedicated accessor function.Adding a dedicated function to check if the stdlib flag is set improves code modularity and readability. This is a good design choice.
239-309:Details
✅ Verification successful
Consider potential overflow with uint8_t for file count.
While the recursive directory traversal is well-implemented with proper error handling, there's a potential issue with the
count_filenamesbeing defined asuint8_t(line 25). This could overflow if more than 255 files are added from the standard library.
🏁 Script executed:
#!/bin/bash # Count how many .dg files are in the standard library # to check if we might exceed uint8_t capacity echo "Checking standard library file count..." find ./stdlib -name "*.dg" | wc -l # Also check Flags struct definition grep -A10 "struct Flags" compiler/main/cli/flags/flags.h || grep -A10 "struct Flags" compiler/main/cli/flags/flags.cLength of output: 1313
STD Lib File Count Validated – No Overflow Risk Detected
- Verified that the standard library contains only 16
.dgfiles, which is well below the 255-file limit of auint8_t.- No changes are needed for the current file-count tracking.
Let's hope these scripts still work lol.
pointbazaar
force-pushed
the
compile-stdlib
branch
from
30b7239 to
08ae42a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
syntax-tools/batcat/install.sh (1)
11-11: Clarify Symbolic Link Target and Consider Idempotence
The updated symbolic link command now explicitly creates the link in the current directory by using.as the target. To further improve the script, consider adding the-fflag to force the creation of the link if one already exists. This would help ensure idempotence when executing the script multiple times.For example, you might update the command as follows:
-ln -s ~/smalldragon/syntax-tools/sublime-text/dragon.sublime-syntax . +ln -sf ~/smalldragon/syntax-tools/sublime-text/dragon.sublime-syntax ..circleci/config.yml (1)
134-145: New Shell Script Check Job AddedThe addition of the
check-shell-scriptsjob is a valuable enhancement. It cleanly checks out the code, updates the package manager, installsshellcheck, and then uses a well-craftedfindcommand to runshellcheckon all shell scripts while excluding directories that should be omitted.Consider pinning the Docker image version (for example, using a specific tag like
library/archlinux:2023.10.05instead oflatest) to improve build reproducibility and avoid unexpected environment changes.Example diff snippet:
- - image: library/archlinux:latest + - image: library/archlinux:2023.10.05format-code.sh (1)
13-14: Intentional unquoted variable in clang-format call.
The shellcheck disable comment for SC2086 indicates that you expect$SRCto expand to multiple arguments. Verify that this behavior is deliberate and well-documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.circleci/config.yml(1 hunks)check-format.sh(1 hunks)examples/assert_examples.sh(1 hunks)format-code.sh(1 hunks)syntax-tools/batcat/install.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/assert_examples.sh
🔇 Additional comments (9)
syntax-tools/batcat/install.sh (1)
7-7: Enhanced Error Handling for Directory Change
By appending|| exit 1, the script now safely exits if thecdcommand fails. This change improves the robustness of the installation process..circleci/config.yml (1)
153-153: Workflow Integration ApprovedIntegrating the
check-shell-scriptsjob into thebuild-workflowalong with other jobs (likecheck-format,build-job-ubuntu-2004, etc.) is a solid improvement. Make sure that the job ordering aligns with your intended build dependencies.format-code.sh (4)
5-5: Solid quoting for SCRIPT_DIR assignment.
The change to userealpath "$(dirname "$0")"ensures that directory names with spaces or special characters are handled correctly.
7-7: Proper quoting in echo statement.
Using"running from" "$SCRIPT_DIR"prevents word-splitting issues when printing the script’s directory.
9-9: Enhanced safety in find command.
Quoting$SCRIPT_DIRand the file pattern arguments improves robustness when paths contain spaces or unusual characters.
11-11: Correctly quoted SRC echo.
Echoing$SRCwithin quotes helps ensure that the output remains intact and readable even if the variable expands to multiple file paths.check-format.sh (3)
15-15: Improved conditional quoting.
Changing the check toif [ "$dirty" ];helps prevent potential issues if the variable contains spaces or ends up empty.
17-17: Robust output for modified files.
Quoting"${dirty}"in the echo statement ensures that file names with spaces or special characters are printed correctly.
21-21: Accurate diff output quoting.
Using quotes around$diffin the echo statement preserves the integrity of the diff output, avoiding word-splitting artifacts.
Handle each TAC instance explicitly. Print an error for an unhandled case.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
compiler/main/avr_code_gen/cg_avr_basic_block.c (1)
183-183: Unreachable code.This return statement is unreachable since all possible execution paths in the switch statement above return a value.
- return false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compiler/main/avr_code_gen/cg_avr_basic_block.c(5 hunks)tac/tac.h(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
compiler/main/avr_code_gen/cg_avr_basic_block.c (8)
tac/tac.h (2) (2)
tac_dest(99-99)tac_kind(112-112)compiler/main/util/ctx.c (1) (1)
ctx_flags(62-64)compiler/main/util/ctx.h (1) (1)
ctx_flags(16-16)rat/rat.c (1) (1)
rat_ensure_register(230-251)rat/rat.h (1) (1)
rat_ensure_register(59-59)tac/tacbuffer.c (1) (1)
tacbuffer_count(116-118)tac/tacbuffer.h (1) (1)
tacbuffer_count(32-32)tac/tac.c (2) (2)
tac_dest(46-72)tac_kind(128-130)
🔇 Additional comments (6)
tac/tac.h (1)
48-85: Improved clarity with explicit enum values.The code now assigns explicit integer values to all TAC_KIND enum members, which improves readability and maintainability. This is particularly valuable when these enum values are used in switch statements elsewhere in the codebase.
compiler/main/avr_code_gen/cg_avr_basic_block.c (5)
18-22: Good addition of error handling through return values.Converting these functions to return boolean status values instead of void allows for proper error propagation, which is a good practice.
46-56: Enhanced error handling for register allocation.The code now properly checks the return status of
allocate_registersand handles failures appropriately, improving robustness.
88-99: Propagating errors up the call chain.The function now properly checks the success of each register allocation and returns early on failure, which is a good error handling pattern.
164-176: Good handling of TAC kinds that don't require register allocation.The switch statement now explicitly handles TAC kinds that don't require register allocation, which improves code clarity and ensures comprehensive error handling.
178-181: Improved error reporting for unhandled TAC kinds.The default case now properly reports unhandled TAC kinds, which will help with debugging and future development.
Indentation was too much. Break up the function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
compiler/main/cli/flags/flags.c (3)
213-234: Solid implementation offlags_add_filenamewith proper memory management.This function properly handles memory allocation failures both for the realloc and strdup calls. The approach of using a temporary variable (
new_filenames) before assigning toflags->filenamesprevents potential memory leaks if reallocation fails.One small optimization: Consider pre-allocating with a larger chunk size rather than reallocating for each new filename to reduce reallocation frequency.
- flags->capacity_filenames = flags->count_filenames + 1; + // Grow capacity by a larger chunk (e.g., 8) to reduce realloc frequency + if (flags->count_filenames >= flags->capacity_filenames) { + flags->capacity_filenames += 8; // Grow by chunks instead of 1 at a time + char** new_filenames = realloc(flags->filenames, flags->capacity_filenames * sizeof(char*)); + if (!new_filenames) { + return false; + } + flags->filenames = new_filenames; + }
241-298: Well-implemented directory entry processing with proper error handling.The function properly handles different types of directory entries and includes appropriate error checking:
- Skips "." and ".." directory entries
- Checks for memory allocation failures after asprintf
- Verifies file stat operations succeed
- Filters files by the .dg extension
- Has special handling for target-specific directories (avr vs x86)
- Properly frees allocated memory
One suggestion about the logic branches on lines 279-287:
- if ((strcmp(name, "avr") == 0) && flags_x86(flags)) { - // do not include 'avr/' if we are compiling for x86 - return true; - } - - if ((strcmp(name, "avr") != 0) && !flags_x86(flags)) { - // only include 'avr/' if we are compiling for avr - return true; - } + // For x86 target: Skip the AVR directory + // For AVR target: Only include the AVR directory, skip others + if ((strcmp(name, "avr") == 0 && flags_x86(flags)) || + (strcmp(name, "avr") != 0 && !flags_x86(flags))) { + return true; + }This consolidates the logic into a single condition that's easier to follow, though the existing implementation with comments is clear enough.
300-321: Good directory traversal implementation with proper resource management.The function correctly:
- Opens the directory and checks for errors
- Iterates through directory entries
- Tracks success state through the loop
- Properly closes the directory handle even on error paths
- Returns the success state
The error handling could be improved by providing more specific error messages for different failure cases. Consider enhancing the error reporting with more context:
DIR* dir = opendir(base_path); if (!dir) { - perror("Failed to open stdlib directory"); + fprintf(stderr, "Failed to open stdlib directory: %s: %s\n", base_path, strerror(errno)); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
compiler/main/cli/flags/flags.c(6 hunks)
🔇 Additional comments (6)
compiler/main/cli/flags/flags.c (6)
9-11: Appropriate include additions for directory handling.These includes are necessary for the new directory traversal functionality that's being added to support the standard library compilation. Good job including all the required headers for the new functionality.
239-240: Forward declaration looks good.Appropriate forward declaration for the recursive function that will be implemented later in the file.
324-327: Clean implementation of the stdlib path handling.This function provides a simple interface to initiate the recursive directory traversal starting from the standard library path.
329-329: Good updates to themakeFlagsfunction.The function has been updated to:
- Return a pointer instead of a boolean
- Handle the stdlib flag appropriately
- Return NULL on failure
These changes align well with the function's new return type and provide proper error handling.
Also applies to: 360-364, 374-374
385-385: Updated return statement to match new function signature.Correctly updated to return the flags pointer rather than a boolean value.
453-455: Good addition of the stdlib flag check function.This function provides a clean interface to check if the stdlib flag is set, following the pattern of other flag check functions in the file.
Issues found in code review. Since r0 is a valid register, we have to compare >= 0. In that case, the register is valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
compiler/main/avr_code_gen/cg_avr_basic_block.c (1)
183-183: Unnecessary return statementThis return statement appears to be unreachable code since all paths in the switch statement return a value. Consider removing it to avoid confusion.
- return false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
compiler/main/avr_code_gen/cg_avr_basic_block.c(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
compiler/main/avr_code_gen/cg_avr_basic_block.c (7)
tac/tac.h (1) (1)
tac_dest(99-99)compiler/main/util/ctx.c (1) (1)
ctx_flags(62-64)compiler/main/util/ctx.h (1) (1)
ctx_flags(16-16)rat/rat.c (1) (1)
rat_ensure_register(230-251)rat/rat.h (1) (1)
rat_ensure_register(59-59)tac/tacbuffer.c (1) (1)
tacbuffer_count(116-118)tac/tacbuffer.h (1) (1)
tacbuffer_count(32-32)
🔇 Additional comments (6)
compiler/main/avr_code_gen/cg_avr_basic_block.c (6)
18-22: Excellent improvement in function signatures for error handlingThe addition of boolean return types for these functions enhances error detection and handling throughout the code. The documentation comments clearly indicate the purpose of the return values.
52-55: Good error handling implementationAdding proper error handling for register allocation failures is important. The error message and exit strategy are appropriate.
92-96: Proper propagation of error statusThe code correctly checks the return value from
allocate_registers_single_tacand fails early if an error occurs, which is a good practice for error handling.
116-116: Correctly fixed register allocation checksThe comparison with
>= 0is correct sincerat_ensure_registerreturns the register index (which can be 0) or -1 on failure. This properly addresses the previously identified issues.Also applies to: 121-121, 134-134, 141-141, 147-147, 153-153, 161-161
164-176: Complete handling of TAC types not requiring register allocationThe code correctly identifies and handles all TAC types that don't require register allocation, returning true for these cases.
178-180: Good defensive programming with error message for unhandled TAC kindsAdding an error message and failing when encountering unhandled TAC kinds is good practice. This will help identify any missing cases during development.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Refactor