refactor: Remove redundant this when referring to member variables. #60

SharafMohamed · 2024-12-05T16:24:45Z

References

Depends on PR#59.
To review in parallel with PR#59, diff against PR#59 locally. In the repo run:

git fetch upstream pull/59/head:pr-59
git fetch upstream pull/60/head:pr-60
git diff pr-59 pr-60

Description

Old standard in this repository was using this->var to refer to member variables in classes. We switched the standard to use m_var. This PR updates previous code to meet this standard.

using Parser<TypedNfaState, TypedDfaState>::m_lexer; added to the top of Lalr1Parser variables in the Private section. This allows this->m_lexer of the Parser parent class to be used as m_lexer in Lalr1Parser.

Validation performed

Previously existing tests still succeed.

Summary by CodeRabbit

Release Notes

New Features
- Introduced the PrefixTree class for managing prefix tree structures in register data.
- Added the RegisterHandler class for managing registers using a prefix tree.
- Implemented the Dfa and Nfa classes for deterministic and non-deterministic finite automata.
Improvements
- Updated naming conventions across the codebase for clarity and consistency, including renaming RegexNFA to Nfa and RegexDFA to Dfa.
- Enhanced error handling in various classes to ensure robustness.
Testing
- Added unit tests for PrefixTree and RegisterHandler to validate functionality and error handling.
- Expanded test coverage for finite automata implementations.

These changes enhance the application's structure, improve readability, and ensure that new features are thoroughly tested.

Co-authored-by: Lin Zhihao <[email protected]>

…eterministic. This is fine as tags are unique across disjoint subtrees.

Co-authored-by: Lin Zhihao <[email protected]>

…og-surgeon into individual-files

…ypedefs to the top of the file to fix compilation error.

…pp files; Also remove RegexDFA.tpp file.

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (9)

src/log_surgeon/Lalr1Parser.tpp (5)
94-96: Simplify the add_token_chain function by refactoring

In lines 94-96 of the add_token_chain function, constructing the rule_chain manually for each character can be error-prone and hard to maintain. Consider refactoring the code to build the chain using a loop, which will make it more scalable and readable.

Here's an example of how you might refactor the code:
std::unique_ptr<finite_automata::RegexAST<TypedNfaState>> rule_chain =
    std::make_unique<finite_automata::RegexASTLiteral<TypedNfaState>>(chain[0]);

for (uint32_t i = 1; i < chain.size(); i++) {
    std::unique_ptr<finite_automata::RegexASTLiteral<TypedNfaState>> next_char_rule =
        std::make_unique<finite_automata::RegexASTLiteral<TypedNfaState>>(chain[i]);
    rule_chain = std::make_unique<finite_automata::RegexASTCat<TypedNfaState>>(
        std::move(rule_chain), std::move(next_char_rule)
    );
}
196-196: Use false == instead of ! according to coding guidelines

In line 196, you have if (!item_set_ptr->m_closure.insert(*item).second). As per the coding guidelines, prefer false == <expression> rather than !<expression>.

Apply this diff:
-    if (!item_set_ptr->m_closure.insert(*item).second) {
+    if (false == item_set_ptr->m_closure.insert(*item).second) {
Line range hint 223-223: Replace this->m_lexer with m_lexer to follow the new standard

In line 223, you use this->m_lexer.m_symbol_id. Replace this->m_lexer with m_lexer to align with the member variable naming convention.

Apply this diff:
-    if (this->m_lexer.m_symbol_id.find(head) == this->m_lexer.m_symbol_id.end()) {
+    if (m_lexer.m_symbol_id.find(head) == m_lexer.m_symbol_id.end()) {
633-633: Prefer false == accept over !accept

In line 633, you have if (!accept). According to the coding guidelines, prefer using false == accept instead.

Apply this diff:
-    if (!accept) {
+    if (false == accept) {
698-698: Use false == is_accepting instead of !is_accepting

In line 698, within the lambda function, replace if (!is_accepting) with if (false == is_accepting) as per the coding guidelines.

Apply this diff:
-            if (!is_accepting) {
+            if (false == is_accepting) {
src/log_surgeon/Lalr1Parser.cpp (1)
11-14: Update member variable initialization to match new naming conventions

In lines 11-14, in the NonTerminal constructor, you can remove unnecessary qualifiers and adhere to the new m_ member variable notation.

Apply this diff:
-NonTerminal::NonTerminal(Production* p)
-        : m_children_start(m_next_children_start),
-          m_production(p),
-          m_ast(nullptr) {
+NonTerminal::NonTerminal(Production* p)
+        : m_children_start(m_next_children_start),
+          m_production(p),
+          m_ast(nullptr) {
     m_next_children_start += p->m_body.size();
 }
src/log_surgeon/finite_automata/Dfa.hpp (1)
54-71: Provide clarification on UTF-8 handling in get_intersect method

In lines 54-71, the get_intersect method mentions handling UTF-8 transitions but currently only supports byte transitions. Consider implementing UTF-8 handling or clarifying the comment.

Apply this diff to update the TODO comment:
-    // TODO: Handle UTF-8 (multi-byte transitions) as well
+    // Note: Currently, this method handles only byte transitions. UTF-8 support is not implemented yet.
src/log_surgeon/finite_automata/Nfa.hpp (2)
151-151: Consider using a more descriptive lambda parameter name.

The lambda parameter name dest_state could be more descriptive, such as target_state or next_state, to better convey its role in the BFS traversal.
-            = [&state_queue, &visited_states](TypedNfaState const* dest_state) {
+            = [&state_queue, &visited_states](TypedNfaState const* target_state) {
-                  if (visited_states.insert(dest_state).second) {
-                      state_queue.push(dest_state);
+                  if (visited_states.insert(target_state).second) {
+                      state_queue.push(target_state);
                   }
              };
Line range hint 165-167: TODO comment needs attention.

The TODO comment about handling the UTF8 case should be addressed to ensure proper support for UTF-8 characters.

Would you like me to help implement the UTF-8 case handling or create a GitHub issue to track this task?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a346104 and ae64f64.

📒 Files selected for processing (27)

CMakeLists.txt (2 hunks)
examples/intersect-test.cpp (4 hunks)
src/log_surgeon/BufferParser.hpp (2 hunks)
src/log_surgeon/Lalr1Parser.cpp (2 hunks)
src/log_surgeon/Lalr1Parser.hpp (7 hunks)
src/log_surgeon/Lalr1Parser.tpp (20 hunks)
src/log_surgeon/Lexer.hpp (6 hunks)
src/log_surgeon/Lexer.tpp (18 hunks)
src/log_surgeon/LexicalRule.hpp (2 hunks)
src/log_surgeon/LogParser.cpp (5 hunks)
src/log_surgeon/LogParser.hpp (4 hunks)
src/log_surgeon/Parser.hpp (1 hunks)
src/log_surgeon/Parser.tpp (3 hunks)
src/log_surgeon/ReaderParser.hpp (2 hunks)
src/log_surgeon/SchemaParser.cpp (1 hunks)
src/log_surgeon/SchemaParser.hpp (4 hunks)
src/log_surgeon/finite_automata/Dfa.hpp (1 hunks)
src/log_surgeon/finite_automata/DfaState.hpp (1 hunks)
src/log_surgeon/finite_automata/DfaStatePair.hpp (1 hunks)
src/log_surgeon/finite_automata/DfaStateType.hpp (1 hunks)
src/log_surgeon/finite_automata/Nfa.hpp (6 hunks)
src/log_surgeon/finite_automata/NfaState.hpp (6 hunks)
src/log_surgeon/finite_automata/RegexAST.hpp (29 hunks)
src/log_surgeon/finite_automata/TaggedTransition.hpp (5 hunks)
tests/CMakeLists.txt (1 hunks)
tests/test-NFA.cpp (1 hunks)
tests/test-lexer.cpp (2 hunks)

✅ Files skipped from review due to trivial changes (2)

src/log_surgeon/ReaderParser.hpp
src/log_surgeon/BufferParser.hpp

🚧 Files skipped from review as they are similar to previous changes (15)

src/log_surgeon/finite_automata/DfaStateType.hpp
src/log_surgeon/LogParser.hpp
src/log_surgeon/Parser.tpp
src/log_surgeon/finite_automata/DfaStatePair.hpp
tests/CMakeLists.txt
examples/intersect-test.cpp
src/log_surgeon/Parser.hpp
CMakeLists.txt
src/log_surgeon/LexicalRule.hpp
src/log_surgeon/finite_automata/TaggedTransition.hpp
tests/test-NFA.cpp
src/log_surgeon/LogParser.cpp
tests/test-lexer.cpp
src/log_surgeon/Lexer.tpp
src/log_surgeon/finite_automata/RegexAST.hpp

🧰 Additional context used

📓 Path-based instructions (9)

src/log_surgeon/Lalr1Parser.cpp (1)