Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds tests for error handling scenarios and includes some code improvements. The changes focus on expanding test coverage for reader, grouper, and expression parsing components while making minor code quality improvements.
- Added comprehensive error handling tests for reader, grouper, and expression components
- Refactored expression parsing to use more efficient operator matching with arrays
- Fixed code quality issues including using
buffer.data()instead of&buffer[0]and improving include organization
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/reader_tests.cpp | Added tests for file open failure, token dump, and various string parsing error scenarios |
| tests/grouper_tests.cpp | Added tests for grouper error scenarios, missing conditions, and placeholder preservation |
| tests/ast_tests.cpp | Extended test range and adjusted buffer sizes for additional test case |
| tests/arithmetic_tests.cpp | Added tests for ternary expressions and prefix parsing error handling |
| src/reader.cpp | Improved buffer access using buffer.data() instead of &buffer[0] |
| src/grouper.cpp | Added null check for placeholder nodes in identify_subgroup |
| src/expression.cpp | Refactored operator matching using array-based lookup for better performance |
| include/expression.hpp | Updated function signatures to use string_view for better performance |
| data/test12.qc | Added new test data file with complex code structure |
| }; | ||
|
|
||
| for (const auto& c : cases) { | ||
| reader r { const_cast<std::string&>(c.input) }; |
There was a problem hiding this comment.
Using const_cast to remove const qualifier is unsafe and unnecessary. The reader constructor should accept const string& or string_view instead of requiring mutable reference.
| reader r { const_cast<std::string&>(c.input) }; | |
| reader r { c.input }; |
| path_in << "test_data/test" << idx.str() << ".qc"; | ||
| reader r(path_in.str()); | ||
| grouper g { r, 512 }; | ||
| size_t extra = (i == 12 ? 2 : 1); |
There was a problem hiding this comment.
The magic numbers 12 and 2 are unexplained. Consider adding a comment explaining why test case 12 requires different buffer sizing or define named constants.
| size_t extra = (i == 12 ? 2 : 1); | |
| // Test case 12 requires additional buffer space due to its larger size. | |
| const int SPECIAL_TEST_CASE_INDEX = 12; | |
| const int EXTRA_BUFFER_MULTIPLIER = 2; | |
| size_t extra = (i == SPECIAL_TEST_CASE_INDEX ? EXTRA_BUFFER_MULTIPLIER : 1); |
| path_in << "test_data/test" << idx.str() << ".qc"; | ||
| reader r(path_in.str()); | ||
| grouper g { r, 512 }; | ||
| size_t extra = (i == 12 ? 2 : 1); |
There was a problem hiding this comment.
Duplicated logic for handling test case 12 buffer sizing. Consider extracting this into a helper function or constant to avoid code duplication.
| size_t extra = (i == 12 ? 2 : 1); | |
| size_t extra = getExtraBufferMultiplier(i); |
| if (tn->value.kind == token_kind::special_character | ||
| || tn->value.kind == token_kind::separator) { | ||
| token tok; | ||
| static constexpr std::array<std::string_view, 20> multi_ops { |
There was a problem hiding this comment.
The magic number 20 should be removed from the array size declaration. Let the compiler deduce the size to avoid maintenance issues when operators are added or removed.
| static constexpr std::array<std::string_view, 20> multi_ops { | |
| static constexpr std::array<std::string_view, sizeof...(multi_ops)> multi_ops { |
No description provided.