-
Notifications
You must be signed in to change notification settings - Fork 7
Add PDF417 decoder and BigInteger implementation #4
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
Conversation
rzeldent
commented
Jun 28, 2025
- Introduced PDFDecoder.h and ZXBigInteger.cpp/h for handling PDF417 decoding and big integer arithmetic.
- Implemented addition, subtraction, multiplication, and division with remainder for big integers.
- Added Nullable class for handling optional values.
- Ensured proper memory management and error handling in arithmetic operations.
- Included necessary headers and namespaces for functionality.
- Introduced PDFDecoder.h and ZXBigInteger.cpp/h for handling PDF417 decoding and big integer arithmetic. - Implemented addition, subtraction, multiplication, and division with remainder for big integers. - Added Nullable class for handling optional values. - Ensured proper memory management and error handling in arithmetic operations. - Included necessary headers and namespaces for functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a PDF417 decoder and a BigInteger implementation while refactoring QR code version management to use a unified “Type” enum. It includes API adjustments for error handling and naming (e.g. renaming isMicroQRCode to isMicro, and VersionForNumber to ModelX methods) and updates several modules in both the QR and PDF417 directories to improve maintainability and clarity.
Reviewed Changes
Copilot reviewed 180 out of 184 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/zxing/qrcode/QRBitMatrixParser.cpp | Updated ReadVersion to use the new Type-based API and switch-case for proper version retrieval. |
| lib/zxing/pdf417/PDFScanningDecoder.cpp | Adjusted calculation of codeword count and error correction handling. |
| lib/zxing/pdf417/PDFDetector.cpp | Refactored rotation handling in the detection loop with clearer return conditions. |
Comments suppressed due to low confidence (2)
lib/zxing/qrcode/QRBitMatrixParser.cpp:30
- Ensure that the computed version number is validated against the supported range for all QR code types so that the subsequent switch-case in ReadVersion reliably returns a valid Version object.
int number = Version::Number(bitMatrix);
lib/zxing/pdf417/PDFDetector.cpp:344
- [nitpick] It would be helpful to document the behavior of the rotation loop (including how the tryRotate flag affects the detection process and rotation accumulation) to make the logic clearer for future maintainability.
for (int rotate90 = 0; rotate90 <= static_cast<int>(tryRotate); ++rotate90) {
| auto numberOfCodewords = barcodeMatrix[0][1].value(); | ||
| int calculatedNumberOfCodewords = detectionResult.barcodeColumnCount() * detectionResult.barcodeRowCount() - GetNumberOfECCodeWords(detectionResult.barcodeECLevel()); | ||
| if (calculatedNumberOfCodewords < 1 || calculatedNumberOfCodewords > CodewordDecoder::MAX_CODEWORDS_IN_BARCODE) | ||
| calculatedNumberOfCodewords = 0; |
Copilot
AI
Jun 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider explicitly handling the case when the calculated number of codewords is invalid rather than defaulting it to 0; this may improve clarity on error conditions and prevent subtle issues in later decoding steps.
|
Question: this is based on the 2.3.0 release or current HEAD? (I was first a bit baffled by the comment until I understood that this is apparently a Copilot generated comment - which is completely useless and misleading in this case ;)). I assume this PR triggered by this discussion? If you are interested in seeing if there is a path making any changes you did to make this project compile on your platform merged upstream, let me know. |
|
Good question. I'll have to look that up because might have been the develop. The 2.3.0 was released on Jan 1st and a lot of activity on zxing lately (but no release yet)! |