feat: add LiteralTimestamp lowering; improve vector ops (FMA, fast-ma…#1
feat: add LiteralTimestamp lowering; improve vector ops (FMA, fast-ma…#1omsherikar wants to merge 10 commits intomainfrom
Conversation
…th, type checks); lint/type fixes
WalkthroughAdds TIMESTAMP_TYPE and SIZE_T_TYPE, timestamp-literal lowering (parsing/validation into a seven-field i32 struct), native size-type initialization, printf/snprintf/malloc/format helpers, cast-to-string formatting and inline string allocation changes, updates PrintExpr type, and adds tests for casts and timestamp literals. Changes
Sequence Diagram(s)sequenceDiagram
participant TS as LiteralTimestamp node
participant V as LLVMLiteIRVisitor
participant IR as LLVM IR builder
TS->>V: visit(LiteralTimestamp)
Note right of V #F0F4C3: split date/time, reject timezone suffixes
V->>V: parse year,month,day,hour,minute,second\nvalidate via datetime
V->>V: parse fractional seconds → nanos (pad/truncate to 9 digits)
V->>IR: emit constant TIMESTAMP_TYPE {y,m,d,h,min,s,nanos}
IR-->>V: constant pushed to result stack
sequenceDiagram
participant C as Cast node (to STRING/ASCII)
participant V as LLVMLiteIRVisitor
participant F as Format globals / snprintf
participant H as Heap allocator
C->>V: visit(Cast)
V->>V: choose format string by source type\n_normalize_int_for_printf for ints
V->>F: get/create format global
V->>F: call _snprintf_heap(fmt_gv, args)
Note right of F #E6F5FF: snprintf decl + malloc allocate buffer on heap
F->>H: allocate buffer and write formatted bytes
F-->>V: pointer to heap string
V->>C: push formatted string value onto result stack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/irx/builders/llvmliteir.py (1)
1078-1097: Consider adding year validation.The code doesn't validate the year value, which could accept unrealistic values (e.g., negative years or year 999999). Depending on your use case, you might want to add reasonable bounds.
Example:
if not (1 <= year <= 9999): raise Exception( f"LiteralTimestamp: year out of range in '{node.value}'." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/irx/builders/llvmliteir.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
src/irx/builders/llvmliteir.py
1046-1046: Redefinition of unused visit from line 1040
(F811)
1065-1069: Create your own exception
(TRY002)
1065-1069: Avoid specifying long messages outside the exception class
(TRY003)
1073-1076: Create your own exception
(TRY002)
1073-1076: Avoid specifying long messages outside the exception class
(TRY003)
1085-1088: Create your own exception
(TRY002)
1085-1088: Avoid specifying long messages outside the exception class
(TRY003)
1091-1093: Create your own exception
(TRY002)
1091-1093: Avoid specifying long messages outside the exception class
(TRY003)
1095-1097: Create your own exception
(TRY002)
1095-1097: Avoid specifying long messages outside the exception class
(TRY003)
1105-1105: Abstract raise to an inner function
(TRY301)
1105-1105: Avoid specifying long messages outside the exception class
(TRY003)
1117-1121: Create your own exception
(TRY002)
1117-1121: Avoid specifying long messages outside the exception class
(TRY003)
1124-1126: Create your own exception
(TRY002)
1124-1126: Avoid specifying long messages outside the exception class
(TRY003)
1128-1130: Create your own exception
(TRY002)
1128-1130: Avoid specifying long messages outside the exception class
(TRY003)
1132-1134: Create your own exception
(TRY002)
1132-1134: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
src/irx/builders/llvmliteir.py (5)
1045-1069: LGTM: Clear format parsing.The docstring clearly documents the supported formats and the parsing logic correctly handles both 'T' and space separators.
Note: The F811 static analysis warning about function redefinition is a false positive—this is the expected pattern for plum-dispatch overloaded methods.
1071-1076: LGTM: Timezone rejection is clear.The explicit rejection of timezone offsets with a descriptive error message is appropriate, and the detection logic correctly identifies timezone suffixes.
1099-1121: LGTM: Fractional seconds handling is correct.The parsing correctly handles optional fractional seconds, truncates to 9 digits as documented, and pads appropriately to compute nanoseconds.
1123-1134: LGTM: Time component validation is correct.The range checks for hour (0-23), minute (0-59), and second (0-59) are accurate, with clear error messages.
1136-1151: LGTM: LLVM struct construction is correct.The constant struct is properly constructed with all seven i32 fields matching the documented layout, and the result is correctly pushed to the stack.
…Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.0.sdk && /Users/omsherikar/miniforge3/envs/irx/bin/python -m pytest tests/test_timestamp.py -q
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/irx/builders/llvmliteir.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
src/irx/builders/llvmliteir.py
1047-1047: Redefinition of unused visit from line 1041
(F811)
1066-1070: Create your own exception
(TRY002)
1066-1070: Avoid specifying long messages outside the exception class
(TRY003)
1074-1077: Create your own exception
(TRY002)
1074-1077: Avoid specifying long messages outside the exception class
(TRY003)
1080-1080: Redefinition of unused datetime from line 17
Remove definition: datetime
(F811)
1089-1092: Create your own exception
(TRY002)
1089-1092: Avoid specifying long messages outside the exception class
(TRY003)
1094-1097: Create your own exception
(TRY002)
1094-1097: Avoid specifying long messages outside the exception class
(TRY003)
1105-1105: Abstract raise to an inner function
(TRY301)
1105-1105: Avoid specifying long messages outside the exception class
(TRY003)
1117-1121: Create your own exception
(TRY002)
1117-1121: Avoid specifying long messages outside the exception class
(TRY003)
1124-1126: Create your own exception
(TRY002)
1124-1126: Avoid specifying long messages outside the exception class
(TRY003)
1128-1130: Create your own exception
(TRY002)
1128-1130: Avoid specifying long messages outside the exception class
(TRY003)
1132-1134: Create your own exception
(TRY002)
1132-1134: Avoid specifying long messages outside the exception class
(TRY003)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/irx/builders/llvmliteir.py (2)
1072-1077: Consider clarifying the timezone detection logic.The condition
"-" in time_part[2:]skips the first two characters to avoid false positives, but this might be unclear to future maintainers. Consider adding a comment explaining why character position 2 onwards is checked, or use a more explicit pattern match.Example with comment:
- if time_part.endswith("Z") or "+" in time_part or "-" in time_part[2:]: + # Check for timezone: 'Z', '+', or '-' (skip first 2 chars to avoid matching HH:MM minus) + if time_part.endswith("Z") or "+" in time_part or "-" in time_part[2:]:
1098-1120: Time parsing is correct, but consider the fractional seconds truncation behavior.The code silently truncates fractional seconds beyond 9 digits (line 1106). This is reasonable for nanosecond precision, but you might want to add a comment explaining this behavior or consider whether to raise an error when precision would be lost.
Optional: Add a comment to document the truncation:
if len(frac) > 9: + # Truncate to nanosecond precision frac = frac[:9]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/irx/builders/llvmliteir.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
src/irx/builders/llvmliteir.py
1047-1047: Redefinition of unused visit from line 1041
(F811)
1066-1070: Create your own exception
(TRY002)
1066-1070: Avoid specifying long messages outside the exception class
(TRY003)
1074-1077: Create your own exception
(TRY002)
1074-1077: Avoid specifying long messages outside the exception class
(TRY003)
1088-1091: Create your own exception
(TRY002)
1088-1091: Avoid specifying long messages outside the exception class
(TRY003)
1093-1096: Create your own exception
(TRY002)
1093-1096: Avoid specifying long messages outside the exception class
(TRY003)
1104-1104: Abstract raise to an inner function
(TRY301)
1104-1104: Avoid specifying long messages outside the exception class
(TRY003)
1116-1120: Create your own exception
(TRY002)
1116-1120: Avoid specifying long messages outside the exception class
(TRY003)
1123-1125: Create your own exception
(TRY002)
1123-1125: Avoid specifying long messages outside the exception class
(TRY003)
1127-1129: Create your own exception
(TRY002)
1127-1129: Avoid specifying long messages outside the exception class
(TRY003)
1131-1133: Create your own exception
(TRY002)
1131-1133: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
src/irx/builders/llvmliteir.py (6)
17-17: Good fix—module-level datetime import resolves the previous F811 issue.The datetime import is now properly placed at module scope, addressing the redefinition warning from the previous review.
1046-1057: LGTM—clear documentation and method signature.The docstring clearly specifies the struct layout and accepted formats.
1058-1070: LGTM—format detection handles both 'T' and space separators correctly.
1079-1096: LGTM—date validation now correctly handles month-specific limits and leap years.Using
datetime(year, month, day)ensures only valid calendar dates are accepted, properly addressing the previous review concern about accepting invalid dates like February 31.
1122-1133: LGTM—time component validation is correct for standard use cases.The range checks properly validate hour (0-23), minute (0-59), and second (0-59). Note that leap seconds (second=60) are not supported, which is acceptable for most timestamp use cases.
1135-1149: LGTM—LLVM IR struct generation is correct and matches the documented layout.The constant struct properly contains all seven i32 fields (year, month, day, hour, minute, second, nanos) as specified in the docstring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/irx/builders/llvmliteir.py (1)
1046-1156: Excellent implementation! Timestamp lowering is correct and comprehensive.The visitor method properly:
- Parses both
YYYY-MM-DDTHH:MM:SSandYYYY-MM-DD HH:MM:SSformats- Rejects timezone suffixes (Z, +, -)
- Validates real calendar dates using
datetime(correctly handles leap years and month-specific limits)- Handles optional fractional seconds (validates digits, truncates to 9, pads with zeros)
- Validates time component ranges
- Creates a well-documented 7-field i32 struct constant
Note: The Ruff F811 warning about
visitredefinition is a false positive—the@dispatchdecorator pattern intentionally creates multiple overloadedvisitmethods.Optional refactor (low priority): Consider creating custom exception classes to replace the generic
Exceptionusage throughout the method. This would improve error handling clarity and align with Python best practices (Ruff TRY002/TRY003). However, this is purely a code quality improvement and can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/irx/builders/llvmliteir.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
src/irx/builders/llvmliteir.py
1047-1047: Redefinition of unused visit from line 1041
(F811)
1066-1070: Create your own exception
(TRY002)
1066-1070: Avoid specifying long messages outside the exception class
(TRY003)
1074-1077: Create your own exception
(TRY002)
1074-1077: Avoid specifying long messages outside the exception class
(TRY003)
1088-1091: Create your own exception
(TRY002)
1088-1091: Avoid specifying long messages outside the exception class
(TRY003)
1093-1096: Create your own exception
(TRY002)
1093-1096: Avoid specifying long messages outside the exception class
(TRY003)
1110-1110: Abstract raise to an inner function
(TRY301)
1110-1110: Avoid specifying long messages outside the exception class
(TRY003)
1122-1126: Create your own exception
(TRY002)
1122-1126: Avoid specifying long messages outside the exception class
(TRY003)
1129-1131: Create your own exception
(TRY002)
1129-1131: Avoid specifying long messages outside the exception class
(TRY003)
1133-1135: Create your own exception
(TRY002)
1133-1135: Avoid specifying long messages outside the exception class
(TRY003)
1137-1139: Create your own exception
(TRY002)
1137-1139: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
src/irx/builders/llvmliteir.py (1)
8-8: LGTM! Required import for date validation.The module-level
datetimeimport is correctly positioned and used for validating calendar dates at line 1086, properly handling leap years and month-specific day limits.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/irx/builders/llvmliteir.py (2)
1111-1139: LGTM: Time parsing logic.The implementation correctly handles:
- Fractional seconds with proper truncation and padding to nanoseconds
- HH:MM:SS parsing
- Named constants improve code readability
Optional: Consider moving the named constants (NS_DIGITS, MAX_HOUR, etc.) to class or module level if they might be reused elsewhere, though current placement is acceptable for single-method use.
1059-1167: Optional: Ruff style warnings remain.Ruff flags several exception-handling style issues (TRY002, TRY003 about custom exceptions; TRY301 about raise placement). These are style preferences and don't affect functionality. If the project aims to be Ruff-clean, consider addressing them; otherwise, they can be suppressed in the config.
Also note: The F811 warning about
visitredefinition at line 1060 is expected and harmless—it's how plum-dispatch implements multiple dispatch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/irx/builders/llvmliteir.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
src/irx/builders/llvmliteir.py
1060-1060: Redefinition of unused visit from line 1054
(F811)
1079-1083: Create your own exception
(TRY002)
1079-1083: Avoid specifying long messages outside the exception class
(TRY003)
1087-1090: Create your own exception
(TRY002)
1087-1090: Avoid specifying long messages outside the exception class
(TRY003)
1101-1104: Create your own exception
(TRY002)
1101-1104: Avoid specifying long messages outside the exception class
(TRY003)
1106-1109: Create your own exception
(TRY002)
1106-1109: Avoid specifying long messages outside the exception class
(TRY003)
1123-1123: Abstract raise to an inner function
(TRY301)
1123-1123: Avoid specifying long messages outside the exception class
(TRY003)
1135-1139: Create your own exception
(TRY002)
1135-1139: Avoid specifying long messages outside the exception class
(TRY003)
1142-1144: Create your own exception
(TRY002)
1142-1144: Avoid specifying long messages outside the exception class
(TRY003)
1146-1148: Create your own exception
(TRY002)
1146-1148: Avoid specifying long messages outside the exception class
(TRY003)
1150-1152: Create your own exception
(TRY002)
1150-1152: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
src/irx/builders/llvmliteir.py (8)
8-8: LGTM: datetime import correctly placed at module level.The import is properly positioned and used for calendar-aware date validation in the LiteralTimestamp visitor.
49-49: LGTM: TIMESTAMP_TYPE field declaration.Consistent with the existing type field pattern in VariablesLLVM.
165-176: LGTM: TIMESTAMP_TYPE initialization.The 7-field i32 struct correctly represents the timestamp layout (year, month, day, hour, minute, second, nanos) as documented in the visitor method.
1059-1083: LGTM: Format parsing logic.The visitor correctly handles both ISO-like formats (with 'T' or space separator) and provides clear error messages for invalid formats.
1085-1090: LGTM: Timezone rejection logic.The checks correctly reject common timezone formats (Z, +offset, -offset) as documented. The logic is sufficient for the stated requirements.
1092-1109: LGTM: Date validation correctly implemented.Using
datetime(year, month, day)properly validates calendar dates including month-specific day limits and leap years. This addresses the past review feedback.
1141-1152: LGTM: Time component range validation.Hour, minute, and second ranges are correctly validated with clear error messages.
1154-1167: LGTM: Constant struct creation.The timestamp constant is correctly constructed with all 7 fields (year, month, day, hour, minute, second, nanos) and pushed to the result stack following the established pattern.
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 (3)
src/irx/builders/llvmliteir.py(7 hunks)src/irx/system.py(1 hunks)tests/test_cast.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_cast.py (4)
src/irx/system.py (2)
Cast(34-49)PrintExpr(10-31)src/irx/builders/llvmliteir.py (1)
LLVMLiteIR(1919-1953)src/irx/builders/base.py (2)
Builder(52-93)module(74-76)tests/conftest.py (1)
check_result(22-65)
🪛 Ruff (0.14.2)
src/irx/builders/llvmliteir.py
1065-1065: Redefinition of unused visit from line 1059
(F811)
1084-1088: Create your own exception
(TRY002)
1084-1088: Avoid specifying long messages outside the exception class
(TRY003)
1092-1095: Create your own exception
(TRY002)
1092-1095: Avoid specifying long messages outside the exception class
(TRY003)
1106-1109: Create your own exception
(TRY002)
1106-1109: Avoid specifying long messages outside the exception class
(TRY003)
1111-1114: Create your own exception
(TRY002)
1111-1114: Avoid specifying long messages outside the exception class
(TRY003)
1128-1128: Abstract raise to an inner function
(TRY301)
1128-1128: Avoid specifying long messages outside the exception class
(TRY003)
1140-1144: Create your own exception
(TRY002)
1140-1144: Avoid specifying long messages outside the exception class
(TRY003)
1147-1149: Create your own exception
(TRY002)
1147-1149: Avoid specifying long messages outside the exception class
(TRY003)
1151-1153: Create your own exception
(TRY002)
1151-1153: Avoid specifying long messages outside the exception class
(TRY003)
1155-1157: Create your own exception
(TRY002)
1155-1157: Avoid specifying long messages outside the exception class
(TRY003)
1596-1596: Prefer TypeError exception for invalid type
(TRY004)
1596-1596: Create your own exception
(TRY002)
1596-1596: Avoid specifying long messages outside the exception class
(TRY003)
1603-1605: Create your own exception
(TRY002)
1603-1605: Avoid specifying long messages outside the exception class
(TRY003)
| def _normalize_int_for_printf(self, v: ir.Value) -> tuple[ir.Value, str]: | ||
| """Promote/truncate integer to match printf format.""" | ||
| INT64_WIDTH = 64 | ||
| if not isinstance(v.type, ir.IntType): | ||
| raise Exception("Expected integer value") | ||
| w = v.type.width | ||
| if w < INT64_WIDTH: | ||
| arg = self._llvm.ir_builder.sext(v, self._llvm.INT64_TYPE) | ||
| return arg, "%lld" | ||
| if w == INT64_WIDTH: | ||
| return v, "%lld" | ||
| raise Exception( | ||
| "Casting integers wider than 64 bits to string is not supported" | ||
| ) |
There was a problem hiding this comment.
Fix bool-to-string cast returning “-1”.
_normalize_int_for_printf() sign-extends every integer narrower than 64 bits. For an i1/boolean value that means true becomes -1, so casting a boolean to string yields "‑1" instead of "1". Please zero-extend i1 (or otherwise special-case booleans) before formatting so that true prints as 1.
🧰 Tools
🪛 Ruff (0.14.2)
1596-1596: Prefer TypeError exception for invalid type
(TRY004)
1596-1596: Create your own exception
(TRY002)
1596-1596: Avoid specifying long messages outside the exception class
(TRY003)
1603-1605: Create your own exception
(TRY002)
1603-1605: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/irx/builders/llvmliteir.py around lines 1592 to 1605, the function
currently sign-extends all integers narrower than 64 bits which makes i1 true
become -1; change the handling so that if v.type.width == 1 (boolean) you
zero-extend (zext) to INT64_TYPE instead of sign-extending, returning the
zextended value and the same printf format; for other widths < 64 keep the
existing sign-extension (sext) behavior and return as before; leave the equality
and >64 error branches unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/irx/builders/llvmliteir.py (1)
1595-1608: Boolean-to-string cast still yields "-1" for true.The function sign-extends all integers narrower than 64 bits. For an
i1(boolean), this meanstrue(bit value 1) sign-extends to-1, so casting a boolean to string prints"-1"instead of"1".Zero-extend booleans instead of sign-extending:
def _normalize_int_for_printf(self, v: ir.Value) -> tuple[ir.Value, str]: """Promote/truncate integer to match printf format.""" INT64_WIDTH = 64 if not isinstance(v.type, ir.IntType): raise Exception("Expected integer value") w = v.type.width if w < INT64_WIDTH: - arg = self._llvm.ir_builder.sext(v, self._llvm.INT64_TYPE) + # Zero-extend booleans, sign-extend other integers + if w == 1: + arg = self._llvm.ir_builder.zext(v, self._llvm.INT64_TYPE) + else: + arg = self._llvm.ir_builder.sext(v, self._llvm.INT64_TYPE) return arg, "%lld" if w == INT64_WIDTH: return v, "%lld"
🧹 Nitpick comments (1)
src/irx/builders/llvmliteir.py (1)
1087-1160: Consider custom exception classes for better error handling.Multiple validation errors in the timestamp parsing use generic
Exceptionwith inline messages. While functional, Ruff suggests creating custom exception types for better categorization and testing.Example refactor pattern:
class TimestampParseError(ValueError): """Raised when timestamp literal format is invalid.""" pass # Then use: raise TimestampParseError( f"LiteralTimestamp: invalid format '{node.value}'. " "Expected 'YYYY-MM-DDTHH:MM:SS[.fffffffff]'." )This is optional—the current approach with descriptive messages works fine for this use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/irx/builders/llvmliteir.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
src/irx/builders/llvmliteir.py
1068-1068: Redefinition of unused visit from line 1062
(F811)
1087-1091: Create your own exception
(TRY002)
1087-1091: Avoid specifying long messages outside the exception class
(TRY003)
1095-1098: Create your own exception
(TRY002)
1095-1098: Avoid specifying long messages outside the exception class
(TRY003)
1109-1112: Create your own exception
(TRY002)
1109-1112: Avoid specifying long messages outside the exception class
(TRY003)
1114-1117: Create your own exception
(TRY002)
1114-1117: Avoid specifying long messages outside the exception class
(TRY003)
1131-1131: Abstract raise to an inner function
(TRY301)
1131-1131: Avoid specifying long messages outside the exception class
(TRY003)
1143-1147: Create your own exception
(TRY002)
1143-1147: Avoid specifying long messages outside the exception class
(TRY003)
1150-1152: Create your own exception
(TRY002)
1150-1152: Avoid specifying long messages outside the exception class
(TRY003)
1154-1156: Create your own exception
(TRY002)
1154-1156: Avoid specifying long messages outside the exception class
(TRY003)
1158-1160: Create your own exception
(TRY002)
1158-1160: Avoid specifying long messages outside the exception class
(TRY003)
1599-1599: Prefer TypeError exception for invalid type
(TRY004)
1599-1599: Create your own exception
(TRY002)
1599-1599: Avoid specifying long messages outside the exception class
(TRY003)
1606-1608: Create your own exception
(TRY002)
1606-1608: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
src/irx/builders/llvmliteir.py (3)
1067-1175: LiteralTimestamp lowering looks solid.The implementation correctly:
- Validates dates using
datetimeconstructor (addressing previous review feedback)- Handles fractional seconds with proper truncation/padding
- Uses named constants for validation bounds
- Rejects timezone suffixes appropriately
Note: Ruff's F811 warning at line 1068 is a false positive—
plum-dispatchallows multiplevisitmethods with different type signatures.
1585-1588: String type mapping is correct.Mapping "string" to "stringascii" for allocation is consistent with how
VariableDeclarationhandles strings elsewhere in the file.
1760-1784: String casting logic correctly covers all numeric types used in the codebase.Verification confirms the implementation handles all integer widths (1, 8, 16, 32, 64 bits) via generic type checking and
_normalize_int_for_printf, which sign-extends smaller integers to 64-bit and explicitly rejects wider integers. All floating types (Float, Double, Half) are covered with proper promotion to double for consistent formatting. No 128-bit integers or unsigned-specific types exist in the codebase, so no additional coverage is needed.
| def _init_native_size_types(self) -> None: | ||
| """Initialize pointer/size_t types from host.""" | ||
| self._llvm.POINTER_BITS = ctypes.sizeof(ctypes.c_void_p) * 8 | ||
| self._llvm.SIZE_T_TYPE = ir.IntType(ctypes.sizeof(ctypes.c_size_t) * 8) |
There was a problem hiding this comment.
_init_native_size_types() is defined but never called.
This method was added to initialize SIZE_T_TYPE based on the host platform, but it's not invoked from initialize() or anywhere else in the code. As a result, SIZE_T_TYPE remains hard-coded to 64-bit at line 184, defeating the purpose of this platform-detection method.
Call this method from initialize() before SIZE_T_TYPE is used:
def initialize(self) -> None:
"""Initialize self."""
self._llvm = VariablesLLVM()
self._llvm.module = ir.module.Module("Arx")
+
+ # Initialize platform-specific types
+ self._init_native_size_types()Then remove the hard-coded line 184 initialization:
- # Platform-sized unsigned integer (assume 64-bit for CI targets)
- self._llvm.SIZE_T_TYPE = ir.IntType(64)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/irx/builders/llvmliteir.py around lines 135-138, the helper
_init_native_size_types() initializes POINTER_BITS and SIZE_T_TYPE for the host
but is never called; update initialize() to call self._init_native_size_types()
before any use of SIZE_T_TYPE (i.e., before the current hard-coded SIZE_T_TYPE
at line 184), and remove the hard-coded 64-bit SIZE_T_TYPE assignment at line
184 so the runtime-detected values are used instead.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/irx/builders/llvmliteir.py (1)
1598-1611: Bool-to-string cast still returns "-1" instead of "1".Sign-extending
i1true (bit=1) toi64yields-1because the sign bit propagates. A past review flagged this, but the code still usessextunconditionally.Apply this diff to zero-extend booleans:
def _normalize_int_for_printf(self, v: ir.Value) -> tuple[ir.Value, str]: """Promote/truncate integer to match printf format.""" INT64_WIDTH = 64 if not isinstance(v.type, ir.IntType): raise Exception("Expected integer value") w = v.type.width if w < INT64_WIDTH: - arg = self._llvm.ir_builder.sext(v, self._llvm.INT64_TYPE) + # Zero-extend i1 (boolean) to avoid true → -1 + if w == 1: + arg = self._llvm.ir_builder.zext(v, self._llvm.INT64_TYPE) + else: + arg = self._llvm.ir_builder.sext(v, self._llvm.INT64_TYPE) return arg, "%lld" if w == INT64_WIDTH: return v, "%lld"
🧹 Nitpick comments (1)
tests/test_literal_timestamp.py (1)
16-18: Regex-based extraction is pragmatic but fragile.The helper parses LLVM IR's string representation to extract field values. While this works for testing, it's brittle—changes to llvmlite's formatting could break it. For production code, consider accessing the constant's operands directly if llvmlite exposes that API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/irx/builders/llvmliteir.py(7 hunks)tests/test_literal_timestamp.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
src/irx/builders/llvmliteir.py
1071-1071: Redefinition of unused visit from line 1065
(F811)
1090-1094: Create your own exception
(TRY002)
1090-1094: Avoid specifying long messages outside the exception class
(TRY003)
1098-1101: Create your own exception
(TRY002)
1098-1101: Avoid specifying long messages outside the exception class
(TRY003)
1112-1115: Create your own exception
(TRY002)
1112-1115: Avoid specifying long messages outside the exception class
(TRY003)
1117-1120: Create your own exception
(TRY002)
1117-1120: Avoid specifying long messages outside the exception class
(TRY003)
1134-1134: Abstract raise to an inner function
(TRY301)
1134-1134: Avoid specifying long messages outside the exception class
(TRY003)
1146-1150: Create your own exception
(TRY002)
1146-1150: Avoid specifying long messages outside the exception class
(TRY003)
1153-1155: Create your own exception
(TRY002)
1153-1155: Avoid specifying long messages outside the exception class
(TRY003)
1157-1159: Create your own exception
(TRY002)
1157-1159: Avoid specifying long messages outside the exception class
(TRY003)
1161-1163: Create your own exception
(TRY002)
1161-1163: Avoid specifying long messages outside the exception class
(TRY003)
1602-1602: Prefer TypeError exception for invalid type
(TRY004)
1602-1602: Create your own exception
(TRY002)
1602-1602: Avoid specifying long messages outside the exception class
(TRY003)
1609-1611: Create your own exception
(TRY002)
1609-1611: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (11)
tests/test_literal_timestamp.py (3)
21-35: LGTM! Basic timestamp lowering test is thorough.The test correctly validates all seven i32 fields, including proper conversion of fractional seconds (
.123→123_000_000nanoseconds).
38-48: LGTM! Fractional truncation test is correct.Properly validates that 10-digit fractional input (
.1234567897) truncates to 9 nanosecond digits and that space separator is accepted.
51-66: LGTM! Error handling tests cover key rejection cases.The tests validate that invalid dates (Feb 30) and timezone-aware timestamps are properly rejected with appropriate error messages.
src/irx/builders/llvmliteir.py (8)
5-5: LGTM! New imports support platform detection and date validation.
ctypesenables runtime pointer-size detection, anddatetimevalidates calendar dates in timestamp literals.Also applies to: 9-9
50-52: LGTM! New type fields properly declared.The additions support timestamp lowering and platform-specific type handling.
136-139: LGTM! Platform-specific type detection is well-implemented.The method correctly uses
ctypesto detect pointer width andsize_tsize at runtime.
145-146: LGTM! Early initialization ensures SIZE_T_TYPE is available.The call placement is correct—after instantiation but before any type usage.
1070-1178: LGTM! Timestamp lowering is thorough and correct.The implementation properly handles:
- Both
Tand space separators- Calendar validation via
datetime()(catches invalid dates like Feb 30)- Fractional second truncation to 9 nanosecond digits
- Time bounds checking with named constants
- Timezone rejection
The Ruff F811 warning at line 1071 is a false positive—plum dispatch allows multiple
visitmethods.
1588-1591: LGTM! String type handling aligns with VariableDeclaration.Using
"stringascii"for inline string variables matches the pattern inVariableDeclarationand ensures consistent pointer-type handling.
1613-1695: LGTM! Printf/malloc helpers are well-implemented.The functions correctly:
- Declare external
mallocandsnprintf- Use
SIZE_T_TYPEfor size parameters- Implement safe heap formatting with two-pass
snprintf(size check, then format)However, these depend on the
SIZE_T_TYPEfix at line 187 to work correctly on non-64-bit platforms.
1763-1787: LGTM! Cast-to-string implementation is sound.The extension properly:
- Handles integer-to-string via
_normalize_int_for_printf- Promotes float/half to double for consistent formatting
- Uses heap allocation for dynamic-length results
Note: Integer casting inherits the boolean sign-extension issue from
_normalize_int_for_printf.
| # Platform-sized unsigned integer (assume 64-bit for CI targets) | ||
| self._llvm.SIZE_T_TYPE = ir.IntType(64) |
There was a problem hiding this comment.
Remove the hard-coded SIZE_T_TYPE initialization.
Line 187 overwrites the platform-detected SIZE_T_TYPE from _init_native_size_types() (line 139), defeating runtime detection. This leftover line should be removed.
Apply this diff:
- # Platform-sized unsigned integer (assume 64-bit for CI targets)
- self._llvm.SIZE_T_TYPE = ir.IntType(64)🤖 Prompt for AI Agents
In src/irx/builders/llvmliteir.py around lines 186 to 187, a hard-coded override
sets self._llvm.SIZE_T_TYPE = ir.IntType(64) which clobbers the
platform-detected SIZE_T_TYPE set by _init_native_size_types() earlier; remove
that line so the runtime-detected SIZE_T_TYPE is preserved (i.e., delete the
assignment at line 187).
omsherikar
force-pushed
the
feature/literal-timestamp-v2
branch
from
8228cd2 to
16864d6
Compare
omsherikar
force-pushed
the
feature/literal-timestamp-v2
branch
from
16864d6 to
583801e
Compare
1 participant
…th, type checks); lint/type fixes
Notes
you work. When you’re ready for a review, change the status to Ready for
review to trigger a new review round. If you make additional changes and
don’t want to trigger the bot, switch the PR back to Draft.
share your feedback; it helps us improve the tool.
as possible to increase the chances of a timely review. Large PRs may not be
reviewed and may be closed.
self-documenting
(guidance).
our Discord to discuss ideas, blockers, or issues
(https://discord.gg/Nu4MdGj9jB).
sensitive data/PII in code, configs, logs, screenshots, or commit history. If
something leaks, rotate the credentials immediately, invalidate the old key,
and note it in the PR so maintainers can assist.
needed for tests, prefer small fixtures or programmatic downloads declared in
makim.yaml (e.g., a task that fetches data at test time). If a large binary is
unavoidable, discuss first and consider Git LFS.
Pull Request description
How to test these changes
...Pull Request checklists
This PR is a:
About this PR:
Author's checklist:
complexity.
Additional information
Reviewer's checklist
Copy and paste this template for your review's note:
Summary by CodeRabbit
New Features
Tests