compiler: initial support for enums#161
Conversation
WalkthroughThe changes add comprehensive support for enumerations throughout the project. New AST structures and fields are introduced to represent enum declarations and members, along with corresponding token kinds and union members. Memory management, visitor traversal, type checking, and TAC generation are updated to recognize and process enum values. Parser modules are extended to parse enum declarations and members, while grammar documentation and tests are revised accordingly. Additionally, a new enum table is implemented and integrated into the symbol table, and lexer token classification is enhanced by introducing a new keyword for enum. Changes
Sequence Diagram(s)sequenceDiagram
participant L as Lexer
participant P as Parser
participant ND as AST (Namespace)
participant ET as EnumTable
participant ST as Symbol Table
L->>P: Provide tokens (including KEYWORD_ENUM and enum value tokens)
P->>P: Call makeEnumDecl() / makeEnumMember() to parse enum declarations
P->>ND: Add parsed EnumDecl to Namespace (update enums, count_enums, capacity_enums)
ND->>P: Return updated Namespace
P->>ST: Pass Namespace during symbol table build
ST->>ET: Call enum_table_fill() with Namespace enums
ET->>ST: Enum entries inserted (using enum_table_insert)
ST->>P: Proceed with further compilation steps
sequenceDiagram
participant TAC as TAC Generator
participant T as Term Processor
participant ET as EnumTable
participant Buffer as TACBuffer
TAC->>T: Process Term of kind TERM_KIND_ENUM_VALUE
T->>T: Call tac_term_enum_value() with assertions
T->>ET: Lookup enum value by name (enum_table_lookup)
ET-->>T: Return enum's integer value (or error if not found)
T->>Buffer: Append TAC constant with retrieved enum value
Buffer->>T: Confirm constant appended
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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: 8
🔭 Outside diff range comments (2)
parser/main/astnodes/Namespace.c (2)
65-65:⚠️ Potential issueFix implicit function declaration warnings
The pipeline reports implicit declarations of sprintf functions.
The function
asprintfat line 68 is a GNU extension and not part of standard C. Either:
- Include the appropriate header that defines asprintf:
+#define _GNU_SOURCE #include <string.h> #include <stdio.h>
- Or replace it with standard functions:
-asprintf(&(res->name), "%s", name); +res->name = malloc(strlen(name) + 1); +if (!res->name) { + goto error_res_token_path; +} +strcpy(res->name, name);Also applies to: 68-68
100-121:⚠️ Potential issueError handling label order issue
There's an unlabeled goto target at line 110 (
error_res_name) but there's no corresponding label in the code.This could lead to unexpected behavior. Make sure all goto targets are properly defined:
error_res_token_path: free(res->token_path); error_res_src_path: free(res->src_path); -error_res_name: +// error_res_name label is not used free(res->name); error_res_enums: free(res->enums);
🧹 Nitpick comments (18)
ast/ast/ast_enum.h (1)
11-13: Consider adding capacity field for dynamic array managementThe
EnumDeclstructure contains a members array but doesn't include a capacity field. If members are added dynamically, you might need a capacity field to manage memory allocation/reallocation, similar to how other dynamic arrays are managed in the codebase.struct EnumDecl { struct ASTNode super; // must be all uppercase (underscores are ok) char* name; struct EnumMember** members; uint16_t count_members; + size_t capacity_members; };token/TokenKeys.h (1)
13-13: Missing documentation for TOKEN_ENUM_VALUEThe new
TOKEN_ENUM_VALUEtoken is added without any descriptive comment. Consider adding a comment to explain its purpose, similar to other token definitions in this file.+// Token for enum values #define TOKEN_ENUM_VALUE 4compiler/main/gen_tac/gen_tac_term.c (1)
44-46: Unnecessary break statement after return.The
breakstatement after thereturnis redundant since the function will exit upon return.case TERM_KIND_ENUM_VALUE: return tac_term_enum_value(buffer, t, ctx); - break;parser/main/astnodes/EnumMember.c (1)
53-59: Consider adding an error message for missing semicolonWhile the code correctly checks for a semicolon, it doesn't provide an error message like in other error cases.
if (next->kind != SEMICOLON) { + fprintf(stderr, "expected ';'\n"); freeTokenListShallow(copy); free(res); return NULL; }tables/enum/enum_table.h (8)
23-29: Insufficient documentation for the return value of enum_table_insertThe function documentation describes the parameters well but doesn't explain what the boolean return value represents. A user might not know that it returns false if the name already exists or if memory allocation fails.
// @brief inserts a new entry into enum value table // @param str the name to enter // @param value the value for that enum value name +// @return true if insertion was successful, false if name is null, already exists, or if memory allocation fails bool enum_table_insert(struct EnumTable* data, char* str, uint64_t value);
27-29: Incorrect documentation format in enum_table_lookupThe documentation states "@returns" twice with contradictory conditions. The second line states "returns integer value >= for the enum value name" which is incomplete and confusing.
-// @returns -1 if entry is not found -// @returns integer value >= for the enum value name +// @return -1 if entry is not found +// @return non-negative integer value when the enum value name is found int64_t enum_table_lookup(struct EnumTable* data, char* name);
31-31: Missing documentation for enum_table_fill functionThe enum_table_fill function lacks any documentation explaining its purpose and parameters.
+// @brief populates the enum table with entries from a namespace +// @param enum_table the table to populate +// @param ns the namespace containing the enum declarations +// @param debug whether to print debug information +// @return true if all entries were inserted successfully, false otherwise bool enum_table_fill(struct EnumTable* enum_table, struct Namespace* ns, bool debug);
33-33: Missing documentation for enum_table_print functionThe enum_table_print function lacks documentation.
+// @brief prints the contents of the enum table to stdout void enum_table_print(struct EnumTable* enum_table);
35-35: Missing documentation for int_value_from_const functionThe int_value_from_const function lacks documentation explaining its purpose, parameter, and possible return values.
+// @brief converts a ConstValue to an integer +// @param cv the ConstValue to convert +// @return the integer value of the constant, or -1 if the kind is not recognized int int_value_from_const(struct ConstValue* cv);
1-36: Function parameters should be marked as const when appropriateSeveral function parameters that are not modified should be marked as
constto improve code safety and readability.-bool enum_table_insert(struct EnumTable* data, char* str, uint64_t value); +bool enum_table_insert(struct EnumTable* data, const char* str, uint64_t value); -int64_t enum_table_lookup(struct EnumTable* data, char* name); +int64_t enum_table_lookup(struct EnumTable* data, const char* name); -bool enum_table_fill(struct EnumTable* enum_table, struct Namespace* ns, bool debug); +bool enum_table_fill(struct EnumTable* enum_table, const struct Namespace* ns, bool debug); -int int_value_from_const(struct ConstValue* cv); +int int_value_from_const(const struct ConstValue* cv);
9-9: Structure declaration should include typedefConsider adding a typedef for EnumTable to simplify usage and maintain consistency with C conventions.
-struct EnumTable; +typedef struct EnumTable EnumTable; +struct EnumTable;
11-17: Structure declaration should include typedefConsider adding a typedef for EnumTableEntry to simplify usage and maintain consistency with C conventions.
-struct EnumTableEntry { +typedef struct EnumTableEntry { // needs to be freed char* name; uint64_t value; -}; +} EnumTableEntry;tables/enum/enum_table.c (6)
44-49: Potential integer overflow in resize conditionThe condition
(data->count_entries + 1) >= data->capacitycould lead to an integer overflow ifcount_entriesis close to SIZE_MAX. Consider a safer comparison.- if ((data->count_entries + 1) >= data->capacity) { + if (data->count_entries >= data->capacity) { data->capacity *= 2; data->entries = realloc(data->entries, data->capacity * sizeof(struct EnumTableEntry*)); assert(data->entries); }
102-112: Explicitly define the constant value kindsThe function
int_value_from_constuses magic numbers for the different kinds of constants. Consider defining these as an enum to improve code readability and maintainability.+// Define this in the appropriate header file (perhaps ast/ast.h) +typedef enum { + CONST_BOOL = 1, + CONST_INT = 2, + CONST_CHAR = 3, + CONST_STRING = 4, // Assuming this exists based on the numbering + CONST_HEX = 5, + CONST_BIN = 6 +} ConstValueKind; int int_value_from_const(struct ConstValue* cv) { switch (cv->kind) { - case 1: return (int)cv->ptr.m1_bool_const; - case 2: return (int)cv->ptr.m2_int_const; - case 3: return (int)cv->ptr.m3_char_const; - case 5: return (int)cv->ptr.m5_hex_const; - case 6: return (int)cv->ptr.m5_hex_const; + case CONST_BOOL: return (int)cv->ptr.m1_bool_const; + case CONST_INT: return (int)cv->ptr.m2_int_const; + case CONST_CHAR: return (int)cv->ptr.m3_char_const; + case CONST_HEX: return (int)cv->ptr.m5_hex_const; + case CONST_BIN: return (int)cv->ptr.m6_bin_const; // Assuming this is the correct field } return -1; }
114-130: Add documentation for enum_table_fill_single functionThe
enum_table_fill_singlefunction lacks documentation explaining its purpose and parameters.+/** + * @brief Fills the enum table with entries from a single enum declaration + * @param enum_table the table to populate + * @param ed the enum declaration to process + * @return true if all entries were inserted successfully, false otherwise + */ static bool enum_table_fill_single(struct EnumTable* enum_table, struct EnumDecl* ed) {
165-165: Potential format specifier mismatch in printfThe printf statement uses
%ldto printentry->valuewhich is defined asuint64_t. This could lead to incorrect output or warnings on some platforms. Use the proper format specifier for uint64_t.- printf("%-20s = %10ld\n", entry->name, entry->value); + printf("%-20s = %10" PRIu64 "\n", entry->name, entry->value);And add the necessary include:
+#include <inttypes.h>
84-100: Consider using strdup in enum_table_lookupThe function
enum_table_lookupis comparing strings using strcmp. This is fine, but consider using a hash table for better performance as the number of enum entries grows.The current implementation has O(n) lookup time. A hash table implementation would provide O(1) average lookup time, which could be beneficial for larger enum tables. Consider implementing a hash table-based lookup for better performance.
17-26: Initialize entries array to NULL in constructorThe
enum_table_ctorfunction usescallocfor the entries array, which initializes all entries to NULL (zero). This is good practice, but it would be clearer to handle the error case ofcallocfailing.struct EnumTable* enum_table_ctor() { struct EnumTable* res = calloc(1, sizeof(struct EnumTable)); + if (res == NULL) { + return NULL; + } + res->count_entries = 0; res->capacity = 10; res->entries = calloc(res->capacity, sizeof(struct DataEntry*)); + if (res->entries == NULL) { + free(res); + return NULL; + } return res; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
ast/ast.h(2 hunks)ast/ast/ast_enum.h(1 hunks)ast/ast/ast_expr.h(2 hunks)ast/ast_declare.h(1 hunks)ast/util/free_ast.c(3 hunks)ast/util/free_ast.h(1 hunks)ast/visitor/visitor.c(5 hunks)ast/visitor/visitor.h(1 hunks)compiler/main/compiler.c(1 hunks)compiler/main/gen_tac/gen_tac.c(0 hunks)compiler/main/gen_tac/gen_tac.h(0 hunks)compiler/main/gen_tac/gen_tac_constvalue.c(1 hunks)compiler/main/gen_tac/gen_tac_term.c(1 hunks)compiler/main/typechecker/tc_term.c(1 hunks)compiler/main/typeinference/typeinfer_term.c(1 hunks)compiler/main/util/fill_tables.c(2 hunks)compiler/main/util/fill_tables.h(1 hunks)compiler/test/gen_tac/test_gen_tac_assignstmt.c(1 hunks)compiler/test/gen_tac/test_gen_tac_structdecl.c(2 hunks)compiler/test/gen_tac/test_gen_tac_variable.c(1 hunks)compiler/test/typeinference/test-src/infer_type_call_with_struct_member_access.dg(1 hunks)docs/html/grammar.html(2 hunks)examples/enum/simple_enum/simple_enum.dg(1 hunks)examples/enum/simple_enum/simple_enum.exitcode(1 hunks)examples/struct/nested/nested.dg(1 hunks)examples/struct/very_nested/very_nested.dg(2 hunks)lexer/src/lexer.c(2 hunks)parser/main/CMakeLists.txt(1 hunks)parser/main/astnodes/EnumDecl.c(1 hunks)parser/main/astnodes/EnumDecl.h(1 hunks)parser/main/astnodes/EnumMember.c(1 hunks)parser/main/astnodes/EnumMember.h(1 hunks)parser/main/astnodes/Namespace.c(6 hunks)parser/main/astnodes/expr/Term.c(1 hunks)tables/CMakeLists.txt(1 hunks)tables/enum/enum_table.c(1 hunks)tables/enum/enum_table.h(1 hunks)tables/symtable/symtable.c(3 hunks)tables/symtable/symtable.h(2 hunks)token/TokenKeys.h(2 hunks)
💤 Files with no reviewable changes (2)
- compiler/main/gen_tac/gen_tac.h
- compiler/main/gen_tac/gen_tac.c
🧰 Additional context used
🧬 Code Graph Analysis (10)
compiler/main/typeinference/typeinfer_term.c (2)
compiler/main/typeinference/util/type_str.h (1)
typeFromStrPrimitive(5-5)compiler/main/typeinference/util/type_str.c (1)
typeFromStrPrimitive(75-83)
parser/main/astnodes/expr/Term.c (2)
token/list/TokenList.h (2)
list_head(30-30)list_consume(16-16)token/list/TokenList.c (2)
list_head(183-185)list_consume(89-93)
compiler/main/gen_tac/gen_tac_term.c (2)
tables/enum/enum_table.h (2)
enum_table_lookup(29-29)enum_table_print(33-33)tables/enum/enum_table.c (2)
enum_table_lookup(84-100)enum_table_print(158-169)
parser/main/astnodes/EnumMember.c (1)
parser/main/astnodes/EnumMember.h (1)
makeEnumMember(6-6)
compiler/main/compiler.c (2)
compiler/main/util/fill_tables.h (1)
fill_tables(8-8)compiler/main/util/fill_tables.c (1)
fill_tables(14-42)
ast/util/free_ast.c (1)
ast/util/free_ast.h (1)
free_enum_decl(32-32)
parser/main/astnodes/EnumDecl.c (3)
parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)parser/main/astnodes/EnumMember.h (1)
makeEnumMember(6-6)parser/main/astnodes/EnumMember.c (1)
makeEnumMember(19-65)
tables/symtable/symtable.c (2)
tables/enum/enum_table.h (2)
enum_table_ctor(19-19)enum_table_dtor(20-20)tables/enum/enum_table.c (2)
enum_table_ctor(17-26)enum_table_dtor(28-38)
parser/main/astnodes/Namespace.c (4)
token/list/TokenList.h (1)
list_head(30-30)token/list/TokenList.c (1)
list_head(183-185)parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)parser/main/astnodes/EnumDecl.c (1)
makeEnumDecl(18-87)
tables/enum/enum_table.c (1)
tables/enum/enum_table.h (7)
enum_table_ctor(19-19)enum_table_dtor(20-20)enum_table_insert(25-25)enum_table_lookup(29-29)int_value_from_const(35-35)enum_table_fill(31-31)enum_table_print(33-33)
🪛 CircleCI: valgrind
parser/main/astnodes/EnumMember.c
[warning] 33-33: Implicit declaration of function 'sprintf'; did you mean 'vsprintf'?
parser/main/astnodes/Namespace.c
[warning] 52-52: Implicit declaration of function 'sprintf'; did you mean 'vsprintf'?
[warning] 68-68: Implicit declaration of function 'sprintf'; did you mean 'vsprintf'?
🔇 Additional comments (77)
examples/enum/simple_enum/simple_enum.exitcode (1)
1-2: Exit code looks appropriate for demonstration purposes.This new file containing just the exit code
28appears to be a simple test case for the enum functionality being added. It's part of thesimple_enumexample, likely corresponding to a value defined in the related.dgfile.parser/main/astnodes/EnumMember.h (1)
1-6: Header file structure looks good.The header file follows good C programming practices with proper include guards, forward declarations, and a clean function signature for creating enum members.
parser/main/astnodes/EnumDecl.h (1)
1-6: Header file structure looks good.The header file follows good C programming practices with proper include guards, forward declarations, and a clean function signature for creating enum declarations.
tables/CMakeLists.txt (1)
17-19: Appropriate integration in build system.The new enum table source files are correctly added to the "sd-tables" static library. The placement and organization fit well with the existing codebase structure.
parser/main/CMakeLists.txt (1)
38-40: Integration of enum support files looks good.The addition of
EnumDecl.candEnumMember.cto the build system is appropriately placed in the CMake configuration. These files will provide the necessary functionality for parsing enum declarations and members.examples/enum/simple_enum/simple_enum.dg (2)
2-6: Enum declaration follows the specified conventions.The enum name
VALUESand its members (VALUE_1,VALUE_28,VALUE_29) are correctly written in uppercase, following the convention mentioned in the PR description. The explicit value assignments are also properly formatted with semicolon separators.
8-11: Example usage of enum value is correct.The main function correctly demonstrates using an enum member (
VALUE_28) as a value that can be returned from a function. This is a good basic example to validate the enum implementation.ast/util/free_ast.h (1)
31-33: Memory management for enum declarations looks good.The addition of the
free_enum_declfunction follows the established pattern for other AST nodes. Each type gets its own free function, ensuring proper memory management. The new section with the comment// enumhelps organize the code by categorizing the enum-related functions.compiler/main/typeinference/typeinfer_term.c (1)
37-37: Type inference for enum values looks goodSetting enum values to be inferred as
uint64is a reasonable choice. This is consistent with the common practice of using integer types for enum values underneath.ast/ast_declare.h (1)
14-14: Forward declaration for EnumDecl correctly addedThe forward declaration for
struct EnumDeclis placed appropriately with other top-level declarations.tables/symtable/symtable.h (2)
9-9: Include for enum table header looks goodThe include statement for the enum table header is appropriately placed with other table include statements.
26-26: EnumTable field added to ST structureThe addition of the
enum_tablefield to theSTstructure allows the symbol table to track enum declarations, which is necessary for the implementation of enum support.compiler/main/typechecker/tc_term.c (1)
37-37: Type checking for enum values looks correctReturning
truefor enum values is consistent with how other constant values are handled in the type checker. Since enum values are essentially named constants, this approach makes sense.parser/main/astnodes/expr/Term.c (1)
68-71: LGTM: Enum value handling added properlyThe implementation for handling enum values in the makeTerm function is correctly structured and follows the pattern established for other token types. The code appropriately sets the Term kind to TERM_KIND_ENUM_VALUE, duplicates the string value using strdup(), and consumes the token from the list.
compiler/main/util/fill_tables.h (2)
3-4: LGTM: Adding stdbool.h header for bool typeAppropriate inclusion of the stdbool.h header to support the updated return type.
8-8: LGTM: Improved error handling with bool return typeGood improvement to change the return type from void to bool, allowing the function to indicate success or failure when filling tables. This is particularly valuable for handling enum table filling failures.
ast/ast/ast_expr.h (2)
81-81: LGTM: Added enum value to TERM_KIND enumAppropriate addition of TERM_KIND_ENUM_VALUE to the enum list to support the new enum term type.
96-96: LGTM: Added enum_value_term to the unionThe char* enum_value_term field is correctly added to the union to store enum value names. This complements the changes in Term.c for handling enum values.
compiler/test/typeinference/test-src/infer_type_call_with_struct_member_access.dg (2)
2-2: LGTM: Improved struct namingGood change to rename the struct from 'A' to the more descriptive 'Astruct', improving code readability.
7-8: LGTM: Updated variable declaration to use renamed structVariable declaration correctly updated to use the renamed Astruct type.
compiler/main/compiler.c (1)
119-121: Good addition of error handling for fill_tablesThe error handling has been improved by checking the return value of
fill_tablesand propagating the error. This ensures that subsequent compilation steps are only executed if the tables are properly filled.lexer/src/lexer.c (2)
336-336: LGTM: Added enum keyword recognitionThe addition of the "enum" keyword recognition is correctly implemented using the existing macro pattern.
354-366: Good implementation of the uppercase naming convention for enum valuesThis change implements the PR requirement that "enum member names must be written in uppercase letters" by checking if all characters in an identifier are uppercase. This helps distinguish enum values from other identifiers in the language.
ast/visitor/visitor.h (1)
27-30: LGTM: Added enum-related node types to the visitor patternThe new node types for enum declarations, members, and values have been appropriately added to the NODE_TYPE enum. This enables the visitor pattern to properly traverse enum-related nodes in the AST.
examples/struct/nested/nested.dg (3)
2-5: Appropriate renaming of struct to distinguish from enumsThe structure has been renamed from
AtoAstructto avoid potential confusion with uppercase-only enum names. This aligns with the PR's naming convention where enum names must be in uppercase.
7-7: Consistent renaming of structThe structure has been renamed from
BtoBstruct, following the same pattern as theAstructrename.
13-13: Updated variable type to match the renamed structThe local variable's type has been correctly updated to use the renamed
Astructtype.ast/ast/ast_enum.h (1)
1-21: Clean implementation of enum structure definitionsThe implementation correctly defines the required structures for enum support with proper inheritance from ASTNode. The comment about uppercase naming convention aligns with the PR objectives.
compiler/test/gen_tac/test_gen_tac_variable.c (1)
37-37: Appropriate structure renaming for clarityRenaming the structure from (likely) "A" to "Astruct" helps distinguish it from enum names, which according to the PR will be uppercase. This change is consistent with the PR objectives.
compiler/test/gen_tac/test_gen_tac_assignstmt.c (1)
57-57: Appropriate structure renaming for clarityRenaming the structure from (likely) "A" to "Astruct" helps distinguish it from enum names, which according to the PR will be uppercase. This change maintains consistency across test files.
compiler/test/gen_tac/test_gen_tac_structdecl.c (1)
15-15: Consistent structure renaming across test casesRenaming the structure from "A" to "Astruct" in both test functions maintains consistency with the changes in other files and helps distinguish struct types from enum types (which will use uppercase names per PR objectives).
Also applies to: 38-38
token/TokenKeys.h (4)
8-8: Value adjustment for TPARAMThe value for
TPARAMhas been decreased from 3 to 2. Ensure all code that directly uses this numerical value is updated accordingly.
11-11: Value adjustment for IDThe value for
IDhas been decreased from 4 to 3. Ensure all code that directly uses this numerical value is updated accordingly.
100-101: LGTM - New enum keyword tokenThe addition of
KEYWORD_ENUMsupports the PR objective of adding enum support to the compiler. The token value is appropriately placed in the keywords section.
4-5:Details
❓ Verification inconclusive
Token value adjustments may impact existing code
The values for
EMPTYandLINE_NOhave been decreased. This change could affect any code that relies on these specific numerical values.
🏁 Script executed:
#!/bin/bash # Check where these token values are used directly in the codebase echo "Checking for direct usage of EMPTY value:" rg -n "== 1" --include="*.c" | grep -i token echo "Checking for direct usage of LINE_NO value:" rg -n "== 2" --include="*.c" | grep -i tokenLength of output: 1167
Attention: Confirm Token Value Modifications
The update in
token/TokenKeys.h(lines 4–5) reduces the numerical values forEMPTY(now 0) andLINE_NO(now 1). This reduction might affect parts of the code relying on the original values.
- Action Required: Manually verify all code paths that reference these definitions.
- Next Step: Use the revised script above (or an equivalent manual search) to check for direct comparisons or usage in C source files.
- Note: The previous attempt using
--include="*.c"resulted in an error. The updated script now uses-g "*.c"for filtering.
🏁 Script executed:
#!/bin/bash # Re-run the check using the proper flag for filtering C source files echo "Checking for direct usage of EMPTY token (value expected == 1):" rg -n "== 1" -g "*.c" | grep -i "token" echo "Checking for direct usage of LINE_NO token (value expected == 2):" rg -n "== 2" -g "*.c" | grep -i "token"Length of output: 725
Verify Token Macro Adjustments and Update Corresponding Test Expectations
The token value changes in
token/TokenKeys.hnow define:#define EMPTY 0 #define LINE_NO 1However, several tests still compare against the previous hardcoded values. For example:
- In
lexer/test/testcases/tests_mixed.c:
- Line 271 asserts that
tokens[0]->line_num == 1- Line 174 asserts that
tokens[1]->line_num == 2- Other tests in
token/test/testcases/tests_tokenlist.candparser/test/astnodes/expr/ExprTest.calso expect numeric values such as1for list sizes.Please carefully verify that these tests are updated to reflect the new token values or that the logic producing the token values is adjusted accordingly. Manual verification of all code paths relying on these macros is recommended.
ast/ast.h (2)
13-13: LGTM - Added include for enum AST definitionsThe inclusion of "ast/ast_enum.h" is appropriate for supporting enum declarations in the AST structure.
43-47: LGTM - New fields for managing enum declarationsThe addition of fields to store enum declarations in the
Namespacestructure follows the same pattern as other declaration types (structs, methods), which is good for consistency.tables/symtable/symtable.c (3)
10-10: LGTM - Added include for enum table supportThe inclusion of the enum table header is appropriate for integrating enum support into the symbol table.
37-37: LGTM - Enum table initializationThe initialization of the enum table in the symbol table constructor is properly implemented, following the pattern used for other table types.
48-48: LGTM - Enum table cleanupThe cleanup of the enum table in the symbol table destructor is properly implemented, ensuring no memory leaks.
examples/struct/very_nested/very_nested.dg (3)
2-2: Struct renaming for clarityThe renaming of structures from single-letter names (
A,B,C) to more descriptive names (As,Bs,Cs) improves code readability and helps distinguish between enum types and struct types.Also applies to: 7-8, 12-12
4-4: Updated struct referenceThe field type has been correctly updated to match the renamed struct.
21-21: Updated local variable typeThe local variable declaration has been correctly updated to use the renamed struct type.
compiler/main/gen_tac/gen_tac_term.c (3)
2-2: Appropriate includes added for the new enum functionality.The addition of these header files is necessary for the enum value handling implementation, providing access to assertion capabilities, symbol tables, and the enum table.
Also applies to: 5-6
10-30: Well-implemented enum value handling function.The new static function properly handles enum value terms with good error checking and debugging output. It:
- Retrieves and validates the symbol table
- Extracts and validates the enum name
- Looks up the enum value and handles errors
- Appends the appropriate TAC constant when successful
34-37: Good defensive programming with parameter validation.Adding assertions for input parameters improves robustness by ensuring invalid inputs are caught early.
ast/visitor/visitor.c (7)
56-56: Good addition of visitor function prototype for enum values.The function declaration follows the established pattern for visitor functions in this file.
64-66: Well-organized declaration of enum visitor functions.The function prototypes are properly grouped under a descriptive comment and follow the established naming convention.
86-88: Proper integration of enum declarations in namespace visitor.The new loop correctly processes enum declarations within a namespace, following the same pattern as other member types.
103-114: Well-implemented enum declaration visitor.The function properly visits the enum declaration and all its members, maintaining the visitor pattern consistency and error handling approach used throughout the codebase.
116-120: Simple and effective enum member visitor implementation.The visitor function for enum members follows the established pattern and correctly handles visitation of these nodes.
377-379: Appropriate handling of enum values in term visitor.The added case statement properly processes enum value terms by calling the new visitor function.
399-402: Clean implementation of enum value visitor.The visitor function for enum values is simple and follows the established pattern in the codebase.
parser/main/astnodes/EnumDecl.c (4)
1-17: Appropriate includes for the new implementation.The file includes all necessary headers for the implementation of enum declaration parsing.
18-57: Well-structured function setup and initial token parsing.The function properly:
- Validates input parameters
- Creates a working copy of the token list
- Initializes the EnumDecl structure
- Parses the enum keyword and name tokens
- Verifies the opening curly brace
The error handling is consistent with the project's style.
58-74: Effective enum member parsing loop.The implementation correctly handles the iterative parsing of enum members with appropriate error handling and memory management.
76-87: Proper finalization of the parsing process.The function correctly:
- Verifies the closing curly brace
- Updates the original token list
- Frees temporary resources
- Returns the constructed EnumDecl
ast/util/free_ast.c (4)
65-67: Proper cleanup of enum declarations in namespace.The added loop ensures all enum declarations in a namespace are properly freed, preventing memory leaks.
108-115: Well-implemented enum declaration cleanup function.The function correctly frees all resources associated with an enum declaration, including all its members and their names.
140-140: Appropriate handling of enum value terms in free_term.The added case properly frees the memory associated with enum value terms.
142-142: Improved error message with case number.The updated error message now includes the specific term kind value for unhandled cases, which will make debugging easier.
compiler/main/util/fill_tables.c (4)
7-7: Good addition of the enum table headerThis include is necessary for the enum_table_fill function used later in the code.
14-14: Function signature improved to indicate success/failureChanging the return type from
voidtoboolproperly reflects the function's ability to fail, which is important for error handling.Also applies to: 41-41
16-18: Good refactoring of debug flagStoring the debug flag in a local variable is a good practice that improves readability and avoids redundant function calls.
31-33: Proper error handling for enum table fillingThe added check for enum_table_fill's return value with early return on failure improves the robustness of the code.
parser/main/astnodes/EnumMember.c (4)
1-12: Comprehensive includes for the new fileThe file includes all necessary headers for AST operations, token handling, and memory management.
19-23: Good implementation of EnumMember creationThe initialization of the EnumMember structure follows the project's conventions for memory allocation and initialization.
45-51: Good error handling for failed constant value creationThe code properly checks if value creation failed and frees all allocated resources before returning NULL.
61-64: Proper token list managementThe function correctly updates the original token list and frees the copy before returning.
docs/html/grammar.html (3)
38-38: Updated Namespace rule to include EnumDeclThe grammar has been properly updated to include enum declarations as part of namespaces.
42-51: Well-structured grammar for enumerationsThe grammar for enum-related constructs is clear and follows the same formatting as other grammar sections. The rules properly define the expected structure of enums in the language.
160-160: Updated Term rule to include EnumValueThe Term production rule now correctly includes EnumValue as a possible term in expressions.
parser/main/astnodes/Namespace.c (5)
8-8: Added necessary include for enum declarationsThe include for EnumDecl.h is required for the new enum-related functionality.
37-37: Proper initialization for enum tracking variablesThe count_enums and capacity_enums are initialized consistently with other similar counters.
Also applies to: 42-42
52-55: Memory allocation for enums arrayThe memory allocation for enums follows the same pattern as for methods and structs, including proper error handling.
🧰 Tools
🪛 CircleCI: valgrind
[warning] 52-52: Implicit declaration of function 'sprintf'; did you mean 'vsprintf'?
76-85: Implemented enum parsing logicThe enum parsing logic is well-implemented, following the same pattern as struct and method parsing.
111-112: Added proper cleanup for enums arrayThe error handling section now includes freeing the enums array, which prevents memory leaks.
pointbazaar
force-pushed
the
implement-enum
branch
from
e1f9a1b to
f0fce19
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
parser/main/astnodes/EnumMember.c (1)
53-58: Missing error message for semicolon.When checking for a semicolon, if it's not found, the function returns NULL without any error message, unlike other error checks in this function.
next = list_head(copy); if (next->kind != SEMICOLON) { + fprintf(stderr, "expected ';' at the end of enum member\n"); freeTokenListShallow(copy); free(res); return NULL; }parser/main/astnodes/Namespace.c (1)
209-209: Inconsistent array reallocation strategy.For structs, the reallocation strategy directly allocates for (count_structs + 1) without using a capacity variable, while for enums (and methods), a capacity variable is used. This inconsistency could lead to confusion and potential bugs.
-res->structs = realloc(res->structs, sizeof(struct StructDecl*) * (res->count_structs + 1)); +if (res->count_structs >= res->capacity_structs) { + res->capacity_structs *= 2; + res->structs = realloc(res->structs, sizeof(struct StructDecl*) * res->capacity_structs); + if (!res->structs) { + // Handle error + return false; + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
ast/ast.h(2 hunks)ast/ast/ast_enum.h(1 hunks)ast/ast/ast_expr.h(2 hunks)ast/ast_declare.h(1 hunks)ast/util/free_ast.c(4 hunks)ast/util/free_ast.h(1 hunks)ast/visitor/visitor.c(5 hunks)ast/visitor/visitor.h(1 hunks)compiler/main/compiler.c(1 hunks)compiler/main/gen_tac/gen_tac.c(0 hunks)compiler/main/gen_tac/gen_tac.h(0 hunks)compiler/main/gen_tac/gen_tac_constvalue.c(1 hunks)compiler/main/gen_tac/gen_tac_term.c(1 hunks)compiler/main/typechecker/tc_term.c(1 hunks)compiler/main/typeinference/typeinfer_term.c(1 hunks)compiler/main/util/fill_tables.c(2 hunks)compiler/main/util/fill_tables.h(1 hunks)compiler/test/gen_tac/test_gen_tac_assignstmt.c(1 hunks)compiler/test/gen_tac/test_gen_tac_structdecl.c(2 hunks)compiler/test/gen_tac/test_gen_tac_variable.c(1 hunks)compiler/test/typeinference/test-src/infer_type_call_with_struct_member_access.dg(1 hunks)docs/html/grammar.html(2 hunks)examples/enum/simple_enum/simple_enum.dg(1 hunks)examples/enum/simple_enum/simple_enum.exitcode(1 hunks)examples/struct/nested/nested.dg(1 hunks)examples/struct/very_nested/very_nested.dg(2 hunks)lexer/src/lexer.c(2 hunks)parser/main/CMakeLists.txt(1 hunks)parser/main/astnodes/EnumDecl.c(1 hunks)parser/main/astnodes/EnumDecl.h(1 hunks)parser/main/astnodes/EnumMember.c(1 hunks)parser/main/astnodes/EnumMember.h(1 hunks)parser/main/astnodes/Namespace.c(6 hunks)parser/main/astnodes/expr/Term.c(1 hunks)tables/CMakeLists.txt(1 hunks)tables/enum/enum_table.c(1 hunks)tables/enum/enum_table.h(1 hunks)tables/symtable/symtable.c(3 hunks)tables/symtable/symtable.h(2 hunks)token/TokenKeys.h(2 hunks)
💤 Files with no reviewable changes (2)
- compiler/main/gen_tac/gen_tac.h
- compiler/main/gen_tac/gen_tac.c
🚧 Files skipped from review as they are similar to previous changes (33)
- examples/enum/simple_enum/simple_enum.exitcode
- parser/main/astnodes/EnumDecl.h
- examples/enum/simple_enum/simple_enum.dg
- parser/main/astnodes/EnumMember.h
- ast/util/free_ast.h
- ast/ast_declare.h
- tables/CMakeLists.txt
- compiler/main/typechecker/tc_term.c
- compiler/main/compiler.c
- ast/ast/ast_expr.h
- compiler/main/typeinference/typeinfer_term.c
- tables/symtable/symtable.h
- parser/main/CMakeLists.txt
- compiler/main/gen_tac/gen_tac_constvalue.c
- lexer/src/lexer.c
- compiler/test/gen_tac/test_gen_tac_variable.c
- compiler/test/typeinference/test-src/infer_type_call_with_struct_member_access.dg
- compiler/test/gen_tac/test_gen_tac_assignstmt.c
- ast/ast.h
- token/TokenKeys.h
- examples/struct/nested/nested.dg
- compiler/test/gen_tac/test_gen_tac_structdecl.c
- tables/symtable/symtable.c
- compiler/main/gen_tac/gen_tac_term.c
- ast/ast/ast_enum.h
- examples/struct/very_nested/very_nested.dg
- ast/util/free_ast.c
- compiler/main/util/fill_tables.h
- tables/enum/enum_table.h
- tables/enum/enum_table.c
- parser/main/astnodes/expr/Term.c
- ast/visitor/visitor.h
- docs/html/grammar.html
🧰 Additional context used
🧬 Code Graph Analysis (2)
parser/main/astnodes/EnumMember.c (1)
parser/main/astnodes/EnumMember.h (1)
makeEnumMember(6-6)
parser/main/astnodes/EnumDecl.c (3)
parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)parser/main/astnodes/EnumMember.h (1)
makeEnumMember(6-6)parser/main/astnodes/EnumMember.c (1)
makeEnumMember(19-65)
🔇 Additional comments (20)
compiler/main/util/fill_tables.c (5)
7-7: New include for enum table support.The addition of the enum table header is necessary for integrating the new enum functionality into the compiler's symbol tables.
14-14: Method signature updated to return bool.Good change - modifying the function to return a boolean value allows for error propagation when filling tables fails. This enhances error handling in the compiler.
16-18: Refactored debug flag retrieval.Storing the debug flag in a local variable improves code readability and avoids multiple calls to
flags_debug(). This is a good refactoring.
31-33: Added enum table filling with error handling.The code now calls
enum_table_fill()and properly handles its return value, returning false if enum table filling fails. This is consistent with the PR's objective of adding enum support.
41-41: Added success return value.Adding an explicit return statement to indicate successful execution completes the boolean return type implementation.
ast/visitor/visitor.c (7)
56-56: Function declaration for handling enum values.This declaration for
visit_enum_valuefollows the visitor pattern established in the codebase, though it differs slightly by taking a char* parameter rather than a struct pointer like most other visitor functions.
64-66: Well-structured function declarations for handling enum AST nodes.The declarations for
visit_enum_declandvisit_enum_memberfollow the established pattern in the codebase, returning a boolean to indicate success or failure during traversal.
86-88: Proper integration of enum traversal in namespace visitor.The new code correctly iterates over the enums in a namespace, following the same pattern used for structs and methods in the surrounding code.
103-114: Well-implemented enum declaration visitor.The implementation of
visit_enum_declproperly visits each enum member and follows the established pattern by:
- Calling the visitor on the enum declaration node
- Iterating through all enum members
- Handling errors appropriately
- Returning a boolean to indicate success/failure
116-120: Simple but correct enum member visitor implementation.The implementation of
visit_enum_memberis straightforward since it doesn't need to traverse any child nodes beyond calling the visitor function for the enum member itself.
377-379: Added support for enum values in term visitor.The new case for
TERM_KIND_ENUM_VALUEin the switch statement properly calls the appropriate visitor function and follows the pattern established for other term kinds.
399-402: Correct implementation of enum value visitor.The implementation of
visit_enum_valueis simple and correctly calls the visitor function with the appropriate node type.parser/main/astnodes/EnumDecl.c (2)
29-30: Initial memory allocation for enum members.The code properly allocates memory for the enum members array with an initial capacity of 1.
41-47: Enum name validation and storage.The code correctly validates that enum names follow the uppercase convention and duplicates the name to store it in the EnumDecl struct.
parser/main/astnodes/EnumMember.c (2)
33-34: Using asprintf for name duplication.Using asprintf is fine here since the project's build environment supports it (as confirmed in previous comments).
45-51: Proper error handling for makeConstValue failure.The function correctly checks if makeConstValue returns NULL and handles the error by freeing all allocated resources.
parser/main/astnodes/Namespace.c (4)
37-37: Proper initialization of enum counts and capacities.The code correctly initializes the count_enums to 0 and capacity_enums to the same INITIAL_CAPACITY constant used for other collections.
Also applies to: 42-42
52-55: Proper memory allocation and error handling for enums array.The code correctly allocates memory for the enums array based on the capacity and includes appropriate error handling, similar to the handling for methods and structs.
78-85: Enum parsing implementation.The code correctly iterates through tokens to find and parse enum declarations, storing them in the namespace and handling errors appropriately.
111-112: Proper resource cleanup for enums in error case.The error handling code correctly frees the enums array in the error cleanup path.
Simple way to create named values. Enum member names and enum names have to be all uppercase. This separates them from other type names like structs.
pointbazaar
force-pushed
the
implement-enum
branch
from
f0fce19 to
8660359
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
parser/main/astnodes/Namespace.c (1)
100-113:⚠️ Potential issueIncomplete cleanup in error handling.
The error handling section only frees the enums array pointer (line 112) but doesn't free individual enum objects. If makeEnumDecl succeeds for some enums before failing, those successfully created enums would leak memory.
Apply this diff to properly clean up enum objects:
error_res_parse_structs: for (int i = 0; i < res->count_includes; i++) { free(res->includes[i]); } free(res->includes); error_res_token_path: free(res->token_path); error_res_src_path: free(res->src_path); error_res_name: free(res->name); error_res_enums: + for (int i = 0; i < res->count_enums; i++) { + free_enum_decl(res->enums[i]); + } free(res->enums);
🧹 Nitpick comments (8)
tables/enum/enum_table.c (5)
41-60: Check for potential overflow when doubling capacityWhile the resizing logic is correct for moderate-sized tables, an extreme number of entries may lead to capacity overflow. Consider checking for
(data->capacity > SIZE_MAX / 2)before doubling.if ((data->count_entries + 1) >= data->capacity) { + if (data->capacity > SIZE_MAX / 2) { + fprintf(stderr, "Error: Enum table capacity overflow\n"); + return false; + } data->capacity *= 2; ... }
62-103: Validate uppercase enforcement and handle partial insertion errors
- The uppercase validation (lines 75-80) correctly rejects any lowercase character. If you plan to allow digits or underscores, consider clarifying or extending the check.
- The insertion logic cleanly checks for duplicates and handles memory failures. Logging additional info before returning
falsecould aid debugging in large projects.
105-121: Potential performance bottleneck for large enums
enum_table_lookupiterates linearly over all entries. While acceptable for small enums, consider a hash-based lookup if enums grow significantly.
123-133: Use of the same pointer for hex and binary valuesBoth cases (5 and 6) map to
cv->ptr.m5_hex_const, which is intentional per your design. Including a brief comment clarifying this shared field could prevent confusion for future maintainers.
179-190: Optional enhancement for printing format
enum_table_printprovides clear output. If you need more advanced formatting or sorting, consider expansions here.parser/main/astnodes/EnumDecl.c (1)
104-107: Improve memory cleanup with dedicated free function.The error handling section directly frees member resources instead of using a dedicated free function for EnumMember (like free_enum_member). This approach might miss freeing other internal resources that EnumMember could contain.
Consider replacing the direct free calls with a proper free_enum_member function:
exit_error: for (size_t i = 0; i < res->count_members; i++) { - free(res->members[i]->name); - free(res->members[i]); + free_enum_member(res->members[i]); } free(res); freeTokenListShallow(copy); return NULL;parser/main/astnodes/Namespace.c (2)
76-86: Extract enum parsing into a separate function.For consistency with structs and methods (which have ns_parse_structs and ns_parse_methods), consider extracting the enum parsing logic into its own function.
Consider creating a function like ns_parse_enums and refactor the code as follows:
+ // @returns false on error + static bool ns_parse_enums(struct Namespace* res, struct TokenList* copy) { + if (list_size(copy) == 0) { return true; } + + struct Token* next = list_head(copy); + + while (next->kind == KEYWORD_ENUM) { + res->enums[res->count_enums] = makeEnumDecl(copy); + if (!res->enums[res->count_enums]) { + return false; + } + res->count_enums++; + + if (res->count_enums >= res->capacity_enums) { + res->capacity_enums *= 2; + struct EnumDecl** new_enums = realloc(res->enums, + sizeof(struct EnumDecl*) * res->capacity_enums); + if (!new_enums) { + return false; + } + res->enums = new_enums; + } + + next = list_head(copy); + } + + return true; + }Then update the makeNamespace function:
- struct Token* next = list_head(copy); - - while (next->kind == KEYWORD_ENUM) { - res->enums[res->count_enums] = makeEnumDecl(copy); - if (!res->enums[res->count_enums]) { - goto error_res_parse_structs; - } - res->count_enums++; - next = list_head(copy); - } + if (!ns_parse_enums(res, copy)) { + goto error_res_parse_structs; + }
109-109: Unused error label in error handling flow.The error label
error_res_name:is included in the error handling flow but is never referenced anywhere in the code (no goto statements jump to this label).Consider removing the unused label or ensuring the error flow properly references it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
ast/ast.h(2 hunks)ast/ast/ast_enum.h(1 hunks)ast/ast/ast_expr.h(2 hunks)ast/ast_declare.h(1 hunks)ast/util/free_ast.c(4 hunks)ast/util/free_ast.h(1 hunks)ast/visitor/visitor.c(5 hunks)ast/visitor/visitor.h(1 hunks)compiler/main/compiler.c(1 hunks)compiler/main/gen_tac/gen_tac.c(0 hunks)compiler/main/gen_tac/gen_tac.h(0 hunks)compiler/main/gen_tac/gen_tac_constvalue.c(1 hunks)compiler/main/gen_tac/gen_tac_term.c(1 hunks)compiler/main/typechecker/tc_term.c(1 hunks)compiler/main/typeinference/typeinfer_term.c(1 hunks)compiler/main/util/fill_tables.c(2 hunks)compiler/main/util/fill_tables.h(1 hunks)compiler/test/gen_tac/test_gen_tac_assignstmt.c(1 hunks)compiler/test/gen_tac/test_gen_tac_structdecl.c(2 hunks)compiler/test/gen_tac/test_gen_tac_variable.c(1 hunks)compiler/test/typeinference/test-src/infer_type_call_with_struct_member_access.dg(1 hunks)docs/html/grammar.html(2 hunks)examples/enum/enum_hex_value/enum_hex_value.dg(1 hunks)examples/enum/enum_hex_value/enum_hex_value.exitcode(1 hunks)examples/enum/simple_enum/simple_enum.dg(1 hunks)examples/enum/simple_enum/simple_enum.exitcode(1 hunks)examples/struct/nested/nested.dg(1 hunks)examples/struct/very_nested/very_nested.dg(2 hunks)lexer/src/lexer.c(2 hunks)parser/main/CMakeLists.txt(1 hunks)parser/main/astnodes/EnumDecl.c(1 hunks)parser/main/astnodes/EnumDecl.h(1 hunks)parser/main/astnodes/EnumMember.c(1 hunks)parser/main/astnodes/EnumMember.h(1 hunks)parser/main/astnodes/Namespace.c(6 hunks)parser/main/astnodes/expr/Term.c(1 hunks)tables/CMakeLists.txt(1 hunks)tables/enum/enum_table.c(1 hunks)tables/enum/enum_table.h(1 hunks)tables/symtable/symtable.c(3 hunks)tables/symtable/symtable.h(2 hunks)token/TokenKeys.h(2 hunks)
💤 Files with no reviewable changes (2)
- compiler/main/gen_tac/gen_tac.h
- compiler/main/gen_tac/gen_tac.c
✅ Files skipped from review due to trivial changes (1)
- examples/enum/enum_hex_value/enum_hex_value.exitcode
🚧 Files skipped from review as they are similar to previous changes (34)
- ast/util/free_ast.h
- examples/enum/simple_enum/simple_enum.exitcode
- compiler/main/gen_tac/gen_tac_constvalue.c
- compiler/main/typechecker/tc_term.c
- parser/main/CMakeLists.txt
- parser/main/astnodes/expr/Term.c
- parser/main/astnodes/EnumDecl.h
- compiler/test/typeinference/test-src/infer_type_call_with_struct_member_access.dg
- compiler/main/typeinference/typeinfer_term.c
- ast/visitor/visitor.h
- compiler/main/util/fill_tables.h
- compiler/test/gen_tac/test_gen_tac_structdecl.c
- ast/ast.h
- compiler/main/compiler.c
- ast/ast_declare.h
- compiler/main/util/fill_tables.c
- parser/main/astnodes/EnumMember.h
- ast/ast/ast_enum.h
- compiler/test/gen_tac/test_gen_tac_assignstmt.c
- tables/symtable/symtable.h
- examples/struct/nested/nested.dg
- compiler/main/gen_tac/gen_tac_term.c
- token/TokenKeys.h
- examples/struct/very_nested/very_nested.dg
- ast/util/free_ast.c
- tables/symtable/symtable.c
- tables/CMakeLists.txt
- examples/enum/simple_enum/simple_enum.dg
- docs/html/grammar.html
- parser/main/astnodes/EnumMember.c
- compiler/test/gen_tac/test_gen_tac_variable.c
- lexer/src/lexer.c
- ast/ast/ast_expr.h
- tables/enum/enum_table.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
tables/enum/enum_table.c (1)
tables/enum/enum_table.h (7)
enum_table_ctor(19-19)enum_table_dtor(20-20)enum_table_insert(25-25)enum_table_lookup(29-29)int_value_from_const(35-35)enum_table_fill(31-31)enum_table_print(33-33)
🔇 Additional comments (13)
tables/enum/enum_table.c (4)
1-7: Use of<ctype.h>is consistent with uppercase checksThe inclusion of
<ctype.h>is appropriate for ensuring uppercase name validation inenum_table_insert.
29-39: Destructor logic looks solidThe destructor properly frees each entry and handles the array and table deallocation. This is straightforward and correct.
135-151: Enum table fill for a single declaration is concise
enum_table_fill_singlecleanly loops over members and inserts each value. This is straightforward, and error conditions are handled properly.
153-177: Filling the table in debug mode is well-structured
enum_table_fillnavigates the enums array and optionally prints debug info. This is well-structured, and the debug toggle is clear.examples/enum/enum_hex_value/enum_hex_value.dg (2)
2-6: Enum definition adheres to uppercase naming conventionUsing uppercase for both the enum name (
VALUES) and the members (VALUE_1,VALUE_6,VALUE_9) meets the PR’s requirement. Good work!
8-11: Return statement from main is clearReturning
VALUE_9demonstrates the correct usage of enum constants, verifying the integration of hex-based enum values.ast/visitor/visitor.c (7)
56-56: New function declaration for visiting enum valuesDeclaring
visit_enum_valueis a clean approach to handle enum values distinctly.
64-67: Forward declarations forvisit_enum_declandvisit_enum_memberThese declarations keep the file organized by allowing usage before the functions are defined.
86-88: Visiting each enum in the namespaceThis loop ensures that all enumerations are processed. It aligns well with the established visitor pattern.
103-114:visit_enum_decleffectively pipes declarations through the visitorIterating over members ensures each enumeration constant is handled, and propagating success/failure addresses potential issues.
116-120:visit_enum_memberproperly invokes the visitorThis function is minimal and consistent. It could be extended if additional logic is needed while visiting enum members.
377-379: New case forTERM_KIND_ENUM_VALUEinvisit_termAdding
visit_enum_valuehere extends the visitor pattern to recognize enum values. This is aligned with your architecture.
399-402:visit_enum_valuevisits the enum name directlyThe straightforward approach passes the enum value string to the visitor. This is a neat, consistent way to handle literals.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
parser/main/astnodes/EnumDecl.c (1)
53-57: Memory leak on early return when enum name is invalid.This repeats a previously identified issue. If
TOKEN_ENUM_VALUEis missing, the function returns without cleaning up resources created before this check.To fix, direct all failures to the
exit_errorlabel:if (list_head(copy)->kind != TOKEN_ENUM_VALUE) { fprintf(stderr, "parsing error, expected enum value (all uppercase) , but got: %s\n", list_code(copy)); - return NULL; + goto exit_error; }
🧹 Nitpick comments (4)
tables/enum/enum_table.c (4)
18-37: Constructor Handles Allocation Properly
The constructor correctly checks forcallocfailures and returnsNULLon error. You could optionally print an error message before returning, to improve observability in case of failures.if (!res) { + fprintf(stderr, "Error: Unable to allocate EnumTable\n"); return NULL; }
51-70: Check for Potential Capacity Overflow
While the resizing logic is valid, consider adding a check for potential overflow when doublingdata->capacity. This prevents scenarios where extremely large values might wrap around. Also, printing an error message before returningfalsecould improve debugging.size_t new_capacity = data->capacity * 2; +if (new_capacity < data->capacity) { + // Overflow occurred + fprintf(stderr, "Error: Enum table capacity overflow\n"); + return false; +} struct EnumTableEntry** new_entries = realloc(data->entries, new_capacity * sizeof(struct EnumTableEntry*)); if (!new_entries) { + fprintf(stderr, "Error: Failed to resize enum table\n"); return false; } data->capacity = new_capacity; data->entries = new_entries;
83-95: Strict Enforcement of Uppercase
Returningfalseon encountering any lowercase character enforces the uppercase-only rule. If you want a more user-friendly approach, you could automatically convert the name to uppercase or issue a specific error message.for (size_t i = 0; i < len; i++) { - if (islower(name[i])) { - return false; - } + if (islower(name[i])) { + // Convert or fail with explicit error + name[i] = toupper(name[i]); + // or + fprintf(stderr, "Error: %s is not uppercase\n", name); + return false; } }
133-143: Consolidate or Document Cases for Hex & Binary
Cases 5 and 6 return the same field (m5_hex_const). Although intentional, consider adding a comment clarifying they share the same union field to avoid confusion.case 5: return (int)cv->ptr.m5_hex_const; case 6: + // Intentional: both hex and binary use the same field return (int)cv->ptr.m5_hex_const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
parser/main/astnodes/EnumDecl.c(1 hunks)parser/main/astnodes/Namespace.c(7 hunks)tables/enum/enum_table.c(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
parser/main/astnodes/Namespace.c (4)
token/list/TokenList.h (1)
list_head(30-30)token/list/TokenList.c (1)
list_head(183-185)parser/main/astnodes/EnumDecl.c (1)
makeEnumDecl(18-113)parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)
parser/main/astnodes/EnumDecl.c (3)
parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)parser/main/astnodes/EnumMember.c (1)
makeEnumMember(19-65)parser/main/astnodes/EnumMember.h (1)
makeEnumMember(6-6)
tables/enum/enum_table.c (1)
tables/enum/enum_table.h (7)
enum_table_ctor(19-19)enum_table_dtor(20-20)enum_table_insert(25-25)enum_table_lookup(29-29)int_value_from_const(35-35)enum_table_fill(31-31)enum_table_print(33-33)
🔇 Additional comments (9)
tables/enum/enum_table.c (8)
1-8: Header Inclusions Look Good
No issues with these includes; they align well with the subsequent usage of ctype, stdlib, etc.
39-49: Destructor Frees Resources Appropriately
This destructor thoroughly frees each entry, its name, and the array itself. No issues found.
72-81: Duplicate Entry Check
Detecting duplicates before insertion and emitting a clear error message is a good approach. This helps maintain data integrity in the enum table.
96-113: Memory Allocation for Entries Is Handled Correctly
Allocating, checking forNULL, and then performingstrdupensures safety. Releasing resources upon failures (e.g.,free(entry)whenstrdupfails) is also a good practice.
115-131: Lookup Function Is Straightforward
This function properly returns the associated value if found, or-1if missing. No concerns with correctness or readability here.
145-161: Filling Single Enum Declaration
Nicely iterates enum members, converts values, and inserts them. If insertion fails, you cleanly returnfalse. This is robust error propagation.
163-187: Filling Enum Table From Namespace
The debug logs and the iteration across all enums are well-structured. Properly returnsfalseon any insertion failure, halting further processing.
189-201: Print Function Is Clear
This function is straightforward and does exactly what it should: printing enum names and values in a neat format.parser/main/astnodes/Namespace.c (1)
124-150: Enums parsing logic looks good.The
ns_parse_enumsfunction correctly resizes theenumsarray, handles error returns, and delegates declaration creation tomakeEnumDecl. The approach is consistent with the existing pattern for methods and structs.
pointbazaar
force-pushed
the
implement-enum
branch
from
3ddd334 to
8426edf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tables/enum/enum_table.c (4)
39-49: Consider checking for null pointers in destructor.
Although the current usage ensures a validdata, adding defensive checks (likeif (!data) return;) would guard against potential misuse. This is a minor improvement for extra safety.void enum_table_dtor(struct EnumTable* data) { + if (!data) { + return; + } for (size_t i = 0; i < data->count_entries; i++) { ...
51-70: Resize function is correct; consider overflow checks.
Doubling the capacity works fine, and the function returnsfalseif reallocation fails. For completeness, you could add a check to ensuredata->capacity * 2does not overflow asize_t. This is generally a minor corner case, so it’s optional.
72-113: Insert function is logically sound; consider logging for lowercase rejection.
- Checking for duplicate names and uppercase naming is done correctly.
- You might want to log an error when a name contains lowercase letters to provide clearer feedback to the caller.
+fprintf(stderr, "Error: enum name %s is not all uppercase\n", name); return false;
115-131: Lookup implementation is straightforward.
Linear search is fine for smaller tables. If performance becomes an issue for large numbers of enum entries, consider a more efficient data structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
parser/main/astnodes/EnumDecl.c(1 hunks)parser/main/astnodes/Namespace.c(7 hunks)tables/enum/enum_table.c(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
parser/main/astnodes/EnumDecl.c (3)
parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)parser/main/astnodes/EnumMember.h (1)
makeEnumMember(6-6)parser/main/astnodes/EnumMember.c (1)
makeEnumMember(19-65)
parser/main/astnodes/Namespace.c (4)
token/list/TokenList.h (1)
list_head(30-30)token/list/TokenList.c (1)
list_head(183-185)parser/main/astnodes/EnumDecl.c (1)
makeEnumDecl(18-112)parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)
tables/enum/enum_table.c (1)
tables/enum/enum_table.h (7)
enum_table_ctor(19-19)enum_table_dtor(20-20)enum_table_insert(25-25)enum_table_lookup(29-29)int_value_from_const(35-35)enum_table_fill(31-31)enum_table_print(33-33)
🔇 Additional comments (7)
tables/enum/enum_table.c (7)
1-9: Headers look good.
The choice of including<assert.h>,<ctype.h>, and other necessary headers is appropriate and minimal.
11-16: Struct definition is clear and concise.
TheEnumTablestruct is straightforward, holding an array of pointers, a count, and a capacity. This is a clear layout for managing dynamically growing entries.
18-37: Robust constructor with allocation checks.
The constructor correctly checks forcallocfailures and handles them gracefully by returningNULL. This ensures that the caller can detect and handle memory allocation issues properly.
133-143: Hex and binary share the same field—verified.
Using the same union field (m5_hex_const) for both hex and binary is intentional in this codebase. The function handles all recognizedkindvalues gracefully.
145-161: Single enum fill routine is clear.
This function properly retrieves each member’s value and inserts it into the table. The short-circuitreturn falseon insert failure is a good practice for error handling.
163-187: Enum table filling logic aligns with PR objectives.
Iterating through each enum declaration and inserting members enforces the uppercase rule (throughenum_table_insert). The debug logs make troubleshooting easier.
189-200: Printing logic is clear and user-friendly.
Displaying each entry name and associated value in a formatted manner is straightforward and effective for debugging.
pointbazaar
force-pushed
the
implement-enum
branch
from
8426edf to
e7cfa1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
parser/main/astnodes/Namespace.c (2)
101-104:⚠️ Potential issueFree the enum name before freeing the enum structure.
Lines 102-103 free
res->enums[i]first, then accessres->enums[i]->name. This is a use-after-free bug. Swap the order to avoid undefined behavior.for (int i = 0; i < res->count_enums; i++) { - free(res->enums[i]->name); - free(res->enums[i]); + free(res->enums[i]->name); + free(res->enums[i]); }
100-112:⚠️ Potential issuePrevent potential double-free of
res->enums.After freeing
res->enumsin theerror_res_parse_enums:block, execution falls through toerror_res_enums:, which freesres->enumsagain. This results in a double-free. To fix this, either remove the free call at lines 101–105 and rely solely on theerror_res_enums:label for cleanup, or jump over theerror_res_enums:label after manually freeingres->enumshere. For example, you can unify the cleanup logic undererror_res_enums::error_res_parse_enums: for (int i = 0; i < res->count_enums; i++) { free(res->enums[i]->name); free(res->enums[i]); } -free(res->enums); -goto error_res_token_path; +goto error_res_enums; error_res_token_path: free(res->token_path); ... error_res_enums: + for (int i = 0; i < res->count_enums; i++) { + free(res->enums[i]->name); + free(res->enums[i]); + } free(res->enums); ...
🧹 Nitpick comments (5)
tables/enum/enum_table.c (5)
18-37: Consider optional logging for allocation failures
The constructor correctly checks for memory allocation failure and returnsNULL. Optionally, a brief log statement before returning could help diagnose errors in production.
51-70: Avoid potential capacity overflow
If this table grows very large, doubling the capacity might cause an overflow. Consider adding a check before multiplying by 2.
72-113: Provide consistent error messages when insert fails
Currently, insertion fails silently on uppercase validation or malloc issues. Displaying an error message could improve debugging.
115-131: Consider more efficient lookups
Linear search is acceptable for small enum tables. For large projects with many enum members, consider a hash map or sorted data structure.
133-143: Use an enum for thecv->kindvalues
The switch statement forcv->kinduses numeric constants. An enumerated type would clarify usage and reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
parser/main/astnodes/EnumDecl.c(1 hunks)parser/main/astnodes/Namespace.c(7 hunks)tables/enum/enum_table.c(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
parser/main/astnodes/EnumDecl.c (3)
parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)parser/main/astnodes/EnumMember.c (1)
makeEnumMember(19-65)parser/main/astnodes/EnumMember.h (1)
makeEnumMember(6-6)
tables/enum/enum_table.c (1)
tables/enum/enum_table.h (7)
enum_table_ctor(19-19)enum_table_dtor(20-20)enum_table_insert(25-25)enum_table_lookup(29-29)int_value_from_const(35-35)enum_table_fill(31-31)enum_table_print(33-33)
parser/main/astnodes/Namespace.c (4)
token/list/TokenList.h (1)
list_head(30-30)token/list/TokenList.c (1)
list_head(183-185)parser/main/astnodes/EnumDecl.c (1)
makeEnumDecl(18-113)parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)
🔇 Additional comments (10)
tables/enum/enum_table.c (6)
1-8: Includes and header references look good
No issues found in the use of standard libraries and header files here.
11-16: Struct definition is concise
Using separate fields forcount_entriesandcapacityis clear and maintainable.
39-49: Destructor implementation is correct
Memory for each entry is freed, followed by the array and the table itself.
145-161: Enum table filling is straightforward
The single-fill function cleanly loops over members, converts values, and inserts them into the enum table.
163-187: Filling multiple enums is well-handled
Debug logs and error propagation are implemented correctly, returningfalseon any insertion failure.
189-200: Printing function is clear
Enumerations are displayed in a readable format. Looks good.parser/main/astnodes/EnumDecl.c (2)
1-17: Well-organized imports and header structure.The file has a clean organization with proper separation between standard library headers, internal project headers, and token-related headers.
18-113: Well-implemented function with proper error handling and memory management.The implementation of
makeEnumDeclfollows good practices:
- Proper input validation before allocating resources
- Well-structured error handling with goto labels for cleanup
- Comprehensive token validation and parsing logic
- Appropriate memory management with allocation checks
- Clean dynamic resizing of the members array
The function correctly parses the enum declaration format while maintaining proper resource management throughout.
parser/main/astnodes/Namespace.c (2)
8-8: Good implementation of enum support infrastructure.The changes properly integrate enum support into the Namespace structure with:
- Appropriate header inclusion
- Initialization of enum count and capacity variables
- Memory allocation for the enum array with proper error handling
The implementation follows the established patterns in the codebase.
Also applies to: 21-21, 39-39, 44-44, 54-57
78-80: Well-implemented enum parsing function.The
ns_parse_enumsfunction:
- Correctly iterates through tokens to find enum declarations
- Properly handles array resizing when needed
- Includes appropriate error checking for memory allocation
- Returns appropriate status values to trigger cleanup on failure
The function follows the same pattern as the existing functions for methods and structs, maintaining consistency in the codebase.
Also applies to: 123-149
Fixes for issues found in code review.
pointbazaar
force-pushed
the
implement-enum
branch
from
e7cfa1e to
ab4826f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
parser/main/astnodes/Namespace.c (1)
129-138: Better memory reallocation pattern than existing code.This reallocation pattern is more robust than the one used in
ns_parse_structs(line 237) as it:
- Checks if capacity will be exceeded before reallocating
- Doubles the capacity rather than just adding one element
- Stores the realloc result in a temporary variable
- Checks if reallocation failed before assigning it back
Consider applying this improved pattern to the structs and methods parsing functions as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
parser/main/astnodes/EnumDecl.c(1 hunks)parser/main/astnodes/Namespace.c(7 hunks)tables/enum/enum_table.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tables/enum/enum_table.c
🧰 Additional context used
🧬 Code Graph Analysis (2)
parser/main/astnodes/EnumDecl.c (3)
parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)parser/main/astnodes/EnumMember.c (1)
makeEnumMember(19-65)parser/main/astnodes/EnumMember.h (1)
makeEnumMember(6-6)
parser/main/astnodes/Namespace.c (4)
token/list/TokenList.h (1)
list_head(30-30)token/list/TokenList.c (1)
list_head(183-185)parser/main/astnodes/EnumDecl.c (1)
makeEnumDecl(18-114)parser/main/astnodes/EnumDecl.h (1)
makeEnumDecl(6-6)
🔇 Additional comments (14)
parser/main/astnodes/EnumDecl.c (6)
24-27: Good use of early error handling and goto pattern.The code correctly handles the case where memory allocation for the EnumDecl structure fails by using the goto pattern, which is an appropriate C idiom for resource cleanup in error paths.
41-45: Good memory allocation check and error handling.The code properly checks if memory allocation for the members array fails and jumps to the appropriate cleanup label. This ensures no memory leaks occur in error conditions.
49-57: Consistent error handling for token validation.The code properly validates that the current token is an enum value (all uppercase as per requirements) and handles the error case correctly by jumping to the appropriate cleanup label.
73-81: Good reallocation pattern with proper error handling.The code uses a safe pattern for array reallocation:
- Storing the realloc result in a temporary variable
- Checking for allocation failure before assigning it back
- Using the appropriate error handling path if allocation fails
This prevents memory leaks or pointer corruption if reallocation fails.
83-88: Robust loop termination condition for enum members.The break condition properly checks if the next token is not an enum value, allowing for graceful exit from the loop without requiring explicit counting or other complex conditions.
102-114: Well-structured cleanup hierarchy with proper resource management.The cleanup code is well-organized with a hierarchical label structure that ensures resources are freed in the correct order. The code properly frees:
- Each enum member and its name
- The members array pointer
- The token list copy
- The main EnumDecl structure
This thorough cleanup approach prevents memory leaks in error scenarios.
parser/main/astnodes/Namespace.c (8)
8-8: Appropriate header inclusion for EnumDecl.The code correctly includes the "EnumDecl.h" header file needed for enum declaration support.
20-21: Consistent function declaration style.The new function declaration for
ns_parse_enumsfollows the same style and documentation pattern as the existingns_parse_methodsandns_parse_structsfunctions, maintaining code consistency.
39-44: Consistent initialization of enum-related fields.The count and capacity fields for enums are initialized following the same pattern as methods and structs, maintaining consistency in the code structure.
54-57: Proper memory allocation with error handling for enums array.The code allocates memory for the enums array and correctly handles allocation failure by jumping to an appropriate error label, consistent with the existing pattern for methods and structs.
78-80: Logical ordering of parsing functions.The enum parsing is called before struct and method parsing, which makes sense from a language structure perspective. The error handling is consistent with the existing pattern.
100-105: Properly ordered memory cleanup for enums.The cleanup code first frees the name and then the enum structure itself for each enum, ensuring proper resource cleanup in case of errors. This follows good C programming practices.
111-112: Consistent cleanup label for enums array.The error_res_enums label frees the enums array, following the same pattern as the existing error labels for methods and structs.
123-149: Well-implemented enum parsing function with array resizing.The
ns_parse_enumsfunction follows a similar pattern to the existing parsing functions but improves on them by implementing dynamic array resizing when the capacity is reached. Key strengths:
- Proper capacity checking and doubling strategy
- Safe reallocation with temporary variable
- Allocation failure handling
- Consistent return value semantics
This is actually an improvement over the struct parsing function (line 237) which doesn't check the reallocation result.
1 participant
Simple way to create named values.
Enum member names and enum names have to be all uppercase.
This separates them from other type names like structs.
Summary by CodeRabbit
New Features
Documentation