-
Notifications
You must be signed in to change notification settings - Fork 6
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
Simplify test-lexer.cpp: #39
Simplify test-lexer.cpp: #39
Conversation
… tag to matching_variable_id; Use full names for vars (r->rule); Clarify if states are NFA or DFA
…for clairty that nothing is shared b/w tests
…iteral arguments; Use const& for non-literals; Use auto where possible; Use uint32_t over int for ids; replace begin() and end() with cbegin() and cend()
…(); Add docstrign to RegexDFAStatePair
…NFA; Made add to nfa functions const
Co-authored-by: Lin Zhihao <[email protected]>
…out-of-order comments); Add comment explaining why <format> isn't sued to cmake
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/test-lexer.cpp (3)
Line range hint
1-3
: LGTM! Consider using fully qualified names instead ofusing
directives.The addition of necessary includes and using directives improves code readability. However, in larger codebases, using directives might lead to name conflicts.
Consider using fully qualified names (e.g.,
std::string
) instead ofusing
directives to prevent potential name conflicts in the future.Also applies to: 12-13
31-33
: LGTM! Consider usingREQUIRE_NOTHROW
more effectively.The changes improve test isolation and code clarity. The use of a new
Schema
object and aconst
variable for the name are good practices.Consider modifying the
REQUIRE_NOTHROW
statement to directly check the type:REQUIRE_NOTHROW(dynamic_cast<RegexASTCatByte&>(*schema_var_ast.m_regex_ptr));This change would make the test more concise and directly assert the expected type.
Also applies to: 40-41, 43-43
96-103
: LGTM! Comprehensive test for complex regex serialization.The new test case effectively validates the serialization of a complex regex AST with named capture groups. This addition improves the overall test coverage of the Schema class.
Consider breaking down the long regex pattern string for better readability:
schema.add_variable( "capture", "Z|(" "A(?<letter>(" "(?<letter1>(a)|(b))|" "(?<letter2>(c)|(d))" "))" "B(?<containerID>\\d+)" "C)", -1 );This change would make the regex pattern easier to understand and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/test-lexer.cpp (4 hunks)
🔇 Additional comments (2)
tests/test-lexer.cpp (2)
25-26
: LGTM! Improved readability.The split of the
RegexASTOrByte
type alias across two lines with proper alignment enhances code readability.
47-47
: LGTM! Improved test isolation and readability.The creation of a new
Schema
object for this test case ensures proper isolation. The simplified dynamic cast improves code readability while maintaining the thorough testing of the AST structure.Also applies to: 56-56
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.
lgtm!
For PR title, how about
Simplify test-lexer.cpp:
- Use more `using` declarations.
- Use a different `Schema` object per test.
- Prettify complex test strings.
Notice that the bullet points will be added to the PR description
using
declarations. - Use a different Schema
object per test. - Prettify complex test strings.
using
declarations. - Use a different Schema
object per test. - Prettify complex test strings.using
declarations.
using
declarations.
using
declarations.Schema
object per test.References
Description
using
to condense code.Schema
object in each test to make it clear there is no interaction b/w the tests.Validation performed