Generate method symbols properly

[Hdt80bro]

Summary by CodeRabbit

• Bug Fixes
  • Improved parsing accuracy for section map data by refining how special character sequences are handled during identifier extraction and storage.

[coderabbitai]

Walkthrough

Modified parse_sect_map to preprocess lines by replacing "::" with "__" before splitting on spaces. This affects .data, .bss, and .rdata section parsing, changing how addresses and names are extracted and stored.

Changes

Cohort / File(s)	Change Summary
Parse section map preprocessing patcher/Patcher.py	Added preprocessing step in three parsing blocks to replace "::" with "__" before splitting line content on spaces in .data*, .bss*, and .rdata sections

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Verify the "::" to "__" replacement logic is semantically correct for the data structures being parsed
• Confirm that the preprocessing step doesn't inadvertently break existing input formats
• Check for potential edge cases where "::" may appear legitimately in address or name fields

Poem

🐰 Two colons become underscores bright,
In parsing blocks, a tidy sight,
Data, BSS, and rdata streams,
Flow smoother through our parsing dreams! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Generate method symbols properly' directly relates to the changes made in parse_sect_map, which improve how method symbols (addresses and names) are extracted and parsed from binary sections.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch Hdt80bro-patch-1

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

patcher/Patcher.py (1)

147-209: Consider adding documentation for the symbol transformation.

While the implementation is correct, consider adding a comment explaining why :: is replaced with __. This would improve maintainability and help future developers understand the purpose of this transformation.

For example, add a comment near the function definition:

def parse_sect_map(file_path: Path) -> dict[str, str]:
    """
    Parse the linker map file to extract symbol addresses.
    
    C++ scoped names (e.g., ClassName::methodName) are transformed to 
    use double underscores (ClassName__methodName) for compatibility
    with the binary patching system.
    """
    addresses: dict[str, str] = {}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0520ff6 and 1ab14db.

📒 Files selected for processing (1)

• patcher/Patcher.py (3 hunks)

⏰ 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: test

🔇 Additional comments (3)

patcher/Patcher.py (3)

173-173: Consistent application of symbol transformation for .data section.

The same :: to __ transformation is correctly applied to .data section parsing. Appropriately omits the ( split since data symbols don't have parameter lists.

---

190-190: Consistent application of symbol transformation for .bss section.

The same :: to __ transformation is correctly applied to .bss section parsing, maintaining consistency across all three parsing blocks (.text, .data, .bss).

Note: The AI summary incorrectly states this affects ".data, .bss, and .rdata" sections. The actual changes affect .text (line 156), .data (line 173), and .bss (line 190) sections.

---

156-156: Now I need to look at how the addresses dictionary is used after being populated by parse_sect_map. Let me search for its downstream usage:

Verify that C++ method symbols with :: are parsed correctly from map files and work properly in downstream consumers.

The .replace("::", "__") normalization appropriately transforms C++ scoped identifiers into valid characters. The split("(")[0] extraction on the .text section correctly strips function parameter info that may appear in map entries.

Before approving, I need to verify the downstream usage of the addresses dictionary. The current review comment asks for verification but should be more specific. Here's the rewritten assessment:

Verify proper handling of normalized symbol names in downstream code.

The change normalizes C++ scoped symbol names by replacing :: with __, which is appropriate for making names compatible with identifier rules used in downstream processing. However, ensure that:

1. All consumers of the addresses dictionary handle the transformed symbol names correctly
2. The normalized names don't cause collisions or mismatches with how symbols are referenced elsewhere
3. Test coverage includes C++ methods with namespace/class scope