parsing FUNCTION block#15
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReworks ComplexParser’s internal type classification (introduces simple_types/simple_types_names and complex_types) and changes how blocks are analyzed and rewritten (iterative multi-phase classification, embedding of complex block lines before function rewrite). Adds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CP as ComplexParser
participant AL as __analyseTypes
participant GL as __getSTLines
participant BL as __getBlockLines
participant B as StructInstance Block
Note over CP: Type classification + ST line extraction
CP->>AL: Run multi-phase analysis -> classify simple/complex types
AL-->>CP: Update simple_types, simple_types_names, complex_types
CP->>GL: Request ST lines for a block
GL->>B: Inspect block
alt block type ∈ complex_types AND ignoreComplexStructs True
GL->>B: Count non-empty, non-_InsertLine lines
alt count > threshold
GL->>BL: __getBlockLines(B)
BL-->>GL: full block lines
GL->>CP: Emit embedded block lines then rewritten function-block
else
GL->>CP: Emit rewritten function-block only
end
else
GL->>CP: Existing handling (use simple_types_names for filtering)
end
sequenceDiagram
autonumber
participant STP as STParser
participant SRC as Source Line
participant REG as Block Registry
Note over STP: New FUNCTION block recognition
STP->>SRC: Read top-level line
alt Line matches "FUNCTION <name> : <return_type>"
STP->>STP: Instantiate `_Function` -> parse name,type,return_type
STP->>REG: Ensure `FUNCTION` present in ALL_BLOCKS & CLOSABLE_BLOCKS
STP-->>STP: Block exposes GetInfo(name,type,return_type)
else
STP->>STP: Defer to other block parsers
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
GlueGenerator.py (1)
26-35: Indexing bug parsing varName; use a regex insteadparts[2] will IndexError for names like "__QX0_1"/"__QW0". Extract kind/sub/indices via regex.
- try: - parts = varName.split("_") - pos1 = int(parts[2][2:]) # number after QX0 or QW0 - pos2 = int(parts[3]) if len(parts) > 3 else 0 - except Exception as e: - raise Exception(f"Error parsing variable name '{varName}': {e}") - - kind = varName[2] # I, Q, M - sub = varName[3] # X, B, W, D, L + m = re.match(r"^__([IQM])([XBWDL])(\d+)(?:_(\d+))?$", varName) + if not m: + raise Exception( + f"Error parsing variable name '{varName}': " + "expected __[IQM][XBWDL]<pos1>[_<pos2>]" + ) + kind, sub = m.group(1), m.group(2) # I/Q/M and X/B/W/D/L + pos1 = int(m.group(3)) + pos2 = int(m.group(4) or 0)
🧹 Nitpick comments (2)
xml2st.py (1)
149-151: Don’t rely on filename “plc.xml” when computing output pathString replacement can mis-save output for non-”plc.xml” inputs. Use directory join.
- st_file = os.path.abspath(args.generate_st).replace("plc.xml", "program.st") + base_dir = os.path.dirname(os.path.abspath(args.generate_st)) + st_file = os.path.join(base_dir, "program.st")GlueGenerator.py (1)
25-25: Reduce noisy stdout in library codeConsider logging at INFO/DEBUG instead of print().
- print(f"Linking variable {varName}") + # logger.debug(f"Linking variable {varName}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
ComplexParser.py(1 hunks)GlueGenerator.py(3 hunks)STParser.py(4 hunks)xml2st.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dcoutinho1328
PR: Autonomy-Logic/xml2st#11
File: ComplexParser.py:214-233
Timestamp: 2025-09-02T21:59:52.612Z
Learning: In the ComplexParser.py __getBlockLines method, the ignoreComplexStructs parameter doesn't need to be propagated in recursive calls because complex structs only appear at the first function call level, not nested within other structures in ST files.
📚 Learning: 2025-09-02T21:59:52.612Z
Learnt from: dcoutinho1328
PR: Autonomy-Logic/xml2st#11
File: ComplexParser.py:214-233
Timestamp: 2025-09-02T21:59:52.612Z
Learning: In the ComplexParser.py __getBlockLines method, the ignoreComplexStructs parameter doesn't need to be propagated in recursive calls because complex structs only appear at the first function call level, not nested within other structures in ST files.
Applied to files:
ComplexParser.py
🧬 Code graph analysis (1)
GlueGenerator.py (1)
util/paths.py (1)
AbsDir(36-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-windows-arm
🔇 Additional comments (2)
xml2st.py (1)
86-88: Formatting-only change: OKThe multi-line endswith() check reads cleaner. No functional impact.
ComplexParser.py (1)
321-334: Verify duplicate/omitted content when embedding TYPE lines before rewritesThe >2 non-empty line gate appends TYPE block lines (excluding struct bodies) before function-block rewrites; otherwise TYPE wrapper is omitted. Please confirm this doesn’t (a) duplicate content across TYPE blocks or (b) drop needed prologue/metadata for short blocks.
Would you like a quick script to diff pre/post output on representative ST files?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ComplexParser.py (3)
325-339: Clarify the complex filtering logic with better comments.This conditional logic that filters non-empty, non-insert lines before extending with block content is hard to follow. The nested
filterandlambdaoperations make the intent unclear.- if ( - len( - list( - filter( - lambda x: not isinstance(x, _InsertLine) - and not EMPTY_LINE.match(x), - block.lines, - ) - ) - ) - > 2 - ): + # Check if block has substantial content (more than just open/close lines) + non_trivial_lines = [ + line for line in block.lines + if not isinstance(line, _InsertLine) and not EMPTY_LINE.match(line) + ] + if len(non_trivial_lines) > 2:
347-355: Consider caching the Jinja environment for performance.Creating a new Jinja
Environmentinstance each time__rewriteStructsAsFunctionBlocksis called introduces unnecessary overhead. The core component of Jinja is the Environment() class. In this example, you create a Jinja environment without any arguments. Later you'll change the parameters of Environment to customize your environment.+ def __init__(self): self.blocks = [] self.arrays = [] self.structs = [] self.programs = [] self.csv_vars = [] self.simple_types = [] self.simple_types_names = [] self.complex_types = [] self.complex_structs = [] self.function_blocks = [] - self.__loader = FileSystemLoader( + self.__jinja_env = Environment(loader=FileSystemLoader( os.path.join(paths.AbsDir(__file__), "templates") - ) + )) def __rewriteStructsAsFunctionBlocks(self): - template = Environment(loader=self.__loader).get_template( + template = self.__jinja_env.get_template( FUNCTION_BLOCK_ST_TEMPLATE )
497-497: Consider caching the Jinja environment instance here as well.Similar to the previous suggestion, creating a new
Environmentinstance in__appendVarsToCSVintroduces unnecessary overhead.template = Environment(loader=self.__loader).get_template(CSV_VARS_TEMPLATE) + template = self.__jinja_env.get_template(CSV_VARS_TEMPLATE)This change would be dependent on implementing the previous suggestion to cache the Jinja environment in
__init__.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ComplexParser.py(7 hunks)
🔇 Additional comments (6)
ComplexParser.py (6)
139-141: LGTM! Improved type classification system.The new type classification system with explicit
simple_types,simple_types_names, andcomplex_typescollections provides better clarity compared to the previousarray_dependanttracking approach.
157-159: LGTM! Consistent clearing of new state variables.The
__clearmethod correctly resets all the new type classification collections to maintain clean state between parser runs.
225-225: LGTM! Improved block complexity determination.Using
complex_typesto determine block complexity in__getBlockLinesis more semantically clear than the previous implementation.
321-321: LGTM! Consistent usage of new naming convention.The change from checking against a previous collection to using
simple_types_namesis consistent with the new type classification system.
371-371: LGTM! Consistent search scope update.The change to search in
simple_typesinstead of the previousarray_dependantis consistent with the new type classification system.
413-413: LGTM! Consistent usage of new type names collection.Using
simple_types_namesfor type checking in__spreadDeclarationsmaintains consistency with the new classification system.
2 participants
Summary by CodeRabbit
New Features
Improvements
Style