-
Notifications
You must be signed in to change notification settings - Fork 129
chore(scripts): add endian converter script #3359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(scripts): add endian converter script #3359
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code ReviewSummaryThis PR adds a utility script for converting numbers between big-endian and little-endian byte order. The script is well-structured and provides clear output formatting. Positive Aspects
Issues & Recommendations1. Logic Error in Conversion (Critical)Location: The conversion logic appears to have an issue: const leBuf = Buffer.allocUnsafe(8);
leBuf.writeBigUInt64LE(num);
const littleEndianValue = leBuf.readBigUInt64BE(); // Read the LE bytes as if they were BEThis code writes a number as little-endian, then reads it back as big-endian. This doesn't actually convert the input number's byte order—it treats the input as if it's already in the desired source format. Expected behavior: If the goal is to show what a big-endian number looks like when interpreted as little-endian (or vice versa), this should be clarified. If the goal is to convert between formats, the logic needs adjustment. Suggested fix: Clarify the intent and align the implementation. The 2. Unused FunctionLocation: The
3. Missing Error HandlingThe script doesn't validate input or handle errors: // Add input validation:
if (process.argv[2] && !/^\d+$/.test(process.argv[2])) {
console.error('Error: Input must be a valid positive integer');
process.exit(1);
}
try {
const num = BigInt(inputNumber);
// ... rest of code
} catch (error) {
console.error('Error: Invalid number format');
process.exit(1);
}4. Unclear Use CaseMissing context: The PR description is empty, making it unclear:
Consider adding:
5. Memory Safety Consideration
Testing Recommendations
Overall AssessmentThe code is clean and readable, but the conversion logic needs clarification or correction. Once the logic issue is resolved and the intent is clear, this will be a useful utility script. Recommendation: Request changes to address the logic issue and add clarifying documentation. Review generated by Claude Code |
fff5961 to
ded7c11
Compare
6f6affa to
7e5b57a
Compare
Code Review: Endian Converter ScriptThanks for adding this utility script! Overall, this is a clean and functional tool for endian conversion. Here are some observations and suggestions: ✅ Positives
🐛 Issues Found1. Logic Inconsistency (lines 7-15 vs 33-36) The // Function interprets input as BE, converts to LE value
function bigEndianToLittleEndian(num: bigint): bigint {
const buffer = Buffer.allocUnsafe(8);
buffer.writeBigUInt64BE(num); // Write as BE
return buffer.readBigUInt64LE(); // Read as LE - gets LE interpretation
}
// Main code interprets input as value, swaps bytes
const leBuf = Buffer.allocUnsafe(8);
leBuf.writeBigUInt64LE(num); // Write as LE
const littleEndianValue = leBuf.readBigUInt64BE(); // Read as BE - swaps bytesThese produce different results. The main code at line 36 performs byte-order swapping by writing with one endianness and reading with another, which is the correct approach for byte swapping. Either:
2. Confusing Variable Names (line 36)
// More accurate name
const swappedValue = leBuf.readBigUInt64BE();3. Missing Error Handling No validation for invalid inputs: const inputNumber = process.argv[2] || '360287970189639680';
try {
const num = BigInt(inputNumber);
// ... rest of code
} catch (error) {
console.error('Error: Invalid number format');
process.exit(1);
}💡 Suggestions1. Add Usage Information if (process.argv.includes('--help') || process.argv.includes('-h')) {
console.log('Usage: endian-converter.ts [number]');
console.log('Converts a number between big and little endian byte orders');
process.exit(0);
}2. Consider Bidirectionality The script title suggests BE→LE only, but the logic actually just swaps bytes (works both ways). Consider:
3. Add Input Validation Check for negative numbers or values that don't fit in 64 bits: if (num < 0n || num > 0xFFFFFFFFFFFFFFFFn) {
console.error('Error: Number must be between 0 and 2^64-1');
process.exit(1);
}🔒 Security
🎯 Performance
📝 DocumentationConsider adding:
Test Coverage❓ No tests included, but acceptable for a utility script in Recommendation: Approve with suggested changes - The script works but would benefit from fixing the unused function and improving error handling. |
Code ReviewThanks for adding this endian converter utility script! Here's my review: Positive Aspects✅ Good structure - Clean, focused utility with clear purpose Issues & Suggestions1. Unused Function (scripts/misc/endian-converter.ts:7-16)The 2. Confusing Conversion Logic (line 36)const littleEndianValue = leBuf.readBigUInt64BE(); // Read the LE bytes as if they were BEThis reads LE bytes as BE, producing a reinterpretation rather than a true endian conversion. The variable name 3. Missing Error HandlingNo validation for invalid input. The 4. Unclear PurposeConsider adding:
Priority Fixes
|
ded7c11 to
e787601
Compare
7e5b57a to
60e4821
Compare
PR Review: Endian Converter ScriptThanks for adding this utility script! Here's my review: Code Quality & Best PracticesPositives:
Issues Found:
RecommendationsFix the conversion logic: // Remove the unused function or use it like this:
const littleEndianValue = bigEndianToLittleEndian(num);
// Or simplify by removing the function and doing it inline:
const leBuf = Buffer.allocUnsafe(8);
leBuf.writeBigUInt64BE(num); // Write as BE
const littleEndianValue = leBuf.readBigUInt64LE(); // Read as LEAdd error handling: if (process.argv.length < 3) {
console.error('Usage: endian-converter.ts <number>');
process.exit(1);
}
let num: bigint;
try {
num = BigInt(inputNumber);
} catch (e) {
console.error(`Error: Invalid number "${inputNumber}"`);
process.exit(1);
}Add a description comment explaining the purpose: /**
* Utility script to convert 64-bit numbers between big-endian and little-endian
* byte representations. Useful for debugging binary data interchange.
*
* Usage: tsx endian-converter.ts <decimal_number>
*/Security Concerns
Performance
Test Coverage
SummaryThe script needs a logic fix to either use the defined conversion function or remove it. The current implementation has a bug where the conversion logic doesn't match the defined helper function. Otherwise, it's a clean utility script that would benefit from better error handling and usage documentation. Severity: Medium - The logic bug should be fixed before merging. |
Code ReviewThanks for adding this endian converter utility script! Overall, the code is clean and functional. Here are some observations and suggestions: ✅ Strengths
🔍 Issues & Suggestions1. Logic Confusion (Bug)Lines 34-36 have a logical issue: const leBuf = Buffer.allocUnsafe(8);
leBuf.writeBigUInt64LE(num);
const littleEndianValue = leBuf.readBigUInt64BE(); // Read the LE bytes as if they were BEThis code writes
Issue: The function Recommendation: Either use the function you defined, or remove it if not needed: const littleEndianValue = bigEndianToLittleEndian(num);2. Security: Buffer.allocUnsafe()Lines 9, 26, and 34 use 3. Input ValidationThe script doesn't validate user input: const inputNumber = process.argv[2] || '360287970189639680';
const num = BigInt(inputNumber);If a user passes an invalid value, it will throw an unhandled error. Consider: try {
const num = BigInt(inputNumber);
} catch (err) {
console.error('Error: Invalid number provided');
process.exit(1);
}4. DocumentationThe top comment only describes converting from big to little endian, but the script actually shows both representations. Consider updating the comment to reflect that it's a conversion tool that displays both byte orders. 5. Minor: Default ValueThe hardcoded default 📝 Test CoverageThis is a standalone utility script, so automated tests may be overkill. However, you could:
SummaryThe script works correctly for its intended purpose, but has one definite bug (unused function) and could benefit from better input validation and clearer documentation. The byte swapping logic itself is sound. Priority fixes:
|
e787601 to
cd9a7ee
Compare
60e4821 to
2cd2955
Compare
Code ReviewI've reviewed the endian converter script. Here's my feedback: ✅ Positive Aspects
🐛 Issues Found1. Logic Bug - Function Not Used (Line 7-16)The Current behavior: Lines 35-36 write a number as little endian, then read those bytes back as big endian - this is the reverse of what the function does and what the title suggests. Recommendation: Either use the defined function or remove it: // Option 1: Use the function
const littleEndianValue = bigEndianToLittleEndian(num);
// Option 2: Remove the unused function and clarify intent2. Confusing Conversion LogicThe conversion logic is unclear:
This doesn't actually convert between endianness - it treats the same number as two different byte orders. Is this intentional? If so, the documentation and variable names should clarify this. Expected behavior for endian conversion: // Input: 0x0500000000000000 (big endian)
// Output: 0x0000000000000005 (little endian - bytes reversed)Current behavior: // Input: 360287970189639680
// Shows the same number in both BE and LE byte representations3. Security: Buffer.allocUnsafe UsageUsing Fix: const buffer = Buffer.alloc(8); // Instead of allocUnsafe📋 Suggestions
try {
const num = BigInt(inputNumber);
} catch (e) {
console.error('Error: Invalid number format');
process.exit(1);
}
/**
* Converts a number from big endian to little endian byte order
*
* Usage:
* ./endian-converter.ts <number>
*
* Example:
* ./endian-converter.ts 360287970189639680
*/
🎯 Priority
SummaryThe script has a significant logic discrepancy between the defined function and the actual implementation. Please clarify the intended behavior and ensure the code matches the documentation. |
Code ReviewOverviewThis PR adds a utility script for converting numbers between big-endian and little-endian byte order. The implementation is straightforward and functional. Issues Found1. Bug: Incorrect Conversion Logic ❗ CRITICALLocation: The conversion logic is incorrect. The current implementation: const leBuf = Buffer.allocUnsafe(8);
leBuf.writeBigUInt64LE(num);
const littleEndianValue = leBuf.readBigUInt64BE(); // Read the LE bytes as if they were BEThis writes the input as LE, then reads it back as BE. This is different from the Issue: If the input is already a big-endian number, writing it with Recommendation: Use the 2. Code Quality: Unused FunctionLocation: The
3. Code Style: Inconsistent with CodebaseThe script uses Recommendation: Consider using Deno for consistency, or document why tsx is preferred for this script. 4. Documentation: Missing PurposeLocation: The comment says "Converts a number from big endian to little endian" but does not explain:
Recommendation: Add JSDoc-style documentation explaining the script purpose and usage. 5. User Experience: No Error HandlingLocation: If invalid input is provided (non-numeric), the script will crash with an unhelpful error. Recommendation: Add input validation with a helpful error message. Positive Aspects ✅
Security Considerations
Testing
SummaryThe main issue is the incorrect conversion logic that makes the script produce unexpected results. Fix the conversion function, remove unused code, and improve documentation. |
cd9a7ee to
f74136c
Compare
2cd2955 to
c74dcd8
Compare
Code Review - PR #3359OverviewThis PR adds a utility script for converting numbers between big endian and little endian byte order. Overall, the script is functional and well-structured for a utility script. Issues & Suggestions🐛 Logic Issue - Incorrect ConversionSeverity: High Lines 34-36 have a logic error. The current implementation: const leBuf = Buffer.allocUnsafe(8);
leBuf.writeBigUInt64LE(num);
const littleEndianValue = leBuf.readBigUInt64BE(); // Read the LE bytes as if they were BEThis writes The script already defines a const littleEndianValue = bigEndianToLittleEndian(num);
const leBuf = Buffer.allocUnsafe(8);
leBuf.writeBigUInt64LE(littleEndianValue);Or if the goal is to show byte reinterpretation, clarify this in comments.
|
c74dcd8 to
a904843
Compare
f74136c to
67ab859
Compare
Code Review: Endian Converter ScriptSummaryThis PR adds a utility script for converting numbers between big endian and little endian byte order. The script is well-structured and functional, but there are a few areas for improvement. ✅ Strengths
🔍 Issues & Recommendations1. Logic Error in Conversion
|
Code ReviewThanks for adding this utility script! Here's my review: Issues Found🐛 Critical: Logic Error in ConversionThe main conversion logic has a bug. On lines 34-35: leBuf.writeBigUInt64LE(num);
const littleEndianValue = leBuf.readBigUInt64BE(); // Read the LE bytes as if they were BEThis doesn't perform endian conversion correctly. You're:
This reinterprets the byte order rather than converting it. The function Expected behavior: Convert a big-endian number to its little-endian representation 💡 Suggested FixEither: Option 1: Use the existing const littleEndianValue = bigEndianToLittleEndian(num);
console.log('\nConverted (Little Endian):');
console.log(` Decimal: ${littleEndianValue}`);
// ... rest of outputOption 2: If the current behavior is intentional (showing byte reinterpretation), rename the function and add documentation explaining this is a byte-order reinterpretation tool, not a mathematical conversion. Minor Issues📝 Naming and Documentation
|
Merge activity
|

No description provided.