labels: add physical address/page operators $$$label and $$$$label, c…#329
labels: add physical address/page operators $$$label and $$$$label, c…#329
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 2 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request adds two new label expression operators ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
sjasm/parser.cpp (1)
84-89: LGTM — ordering is correct.Placing these before the existing
DISP_NONE != PseudoORG && '$' == p[0] && '$' == p[1] && '$' == p[2]branch at line 90 is the right choice: the new branches requireisLabelStart(p+3)/isLabelStart(p+4), so the bare$$$/$$$$operators inside DISP still fall through to the existing handler, and$$<label>at line 100 is unaffected. Buffer reads ofp[1..4]are safe thanks to short-circuit evaluation against the NUL terminator.One minor nit (optional): the four-way
'$' == p[0] && '$' == p[1] && ...chain duplicates logic with line 90; a small helper likedollarRun(p, n)or a precomputed count would read a bit cleaner, but given the localized scope it's fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sjasm/parser.cpp` around lines 84 - 89, Duplicate inline checks for consecutive '$' characters in parser.cpp can be replaced by a small helper to improve readability; add a helper like int dollarRun(const char *p) or bool dollarRun(const char *p, int n) and use it in the branches that currently use chains like '$' == p[0] && '$' == p[1] ... (the branches that call GetLabelPhPage and GetLabelPhValue and the existing DISP_NONE != PseudoORG branch), then update those conditionals to call the helper while preserving the existing isLabelStart(p+3)/isLabelStart(p+4) checks and short-circuit/NUL-safety so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/documentation.xml`:
- Line 213: Update the changelog text to show the label operand by replacing the
zero-operand examples with the operand forms: change the occurrences that
currently read `$$$` and `$$$$` to `$$$label` and `$$$$label` respectively so
the meaning is unambiguous; edit the entry that references the link IDs
op_label_ph_val and op_label_ph_page to use these operand forms.
- Around line 1167-1168: Fix the inconsistent placeholder name by changing the
second entry’s "lb" occurrences to "lab": update the placeholder token $$$$lb to
$$$$lab, the primary token (currently $$$$label) if it contains the short name
to use $$$$label with "lab" as appropriate, and replace the quoted "lb" label
text with "lab" in the indexterm with id "op_label_ph_page" so both entries
consistently use "lab".
In `@tests/devices/get_label_physical_memory_operators.asm`:
- Around line 1-90: The test assembly file
tests/devices/get_label_physical_memory_operators.asm is missing its expected
output artifact; add a companion file with the same stem named either
tests/devices/get_label_physical_memory_operators.lst (if you want to capture
assembler listing/errors) or
tests/devices/get_label_physical_memory_operators.bin (if the asm assembles
successfully) so the test runner can validate output; place the expected listing
or binary corresponding to the assembly that defines labels like
orgL1/dispL1/Equ1/MyS2/MainLab so the test harness can compare results.
---
Nitpick comments:
In `@sjasm/parser.cpp`:
- Around line 84-89: Duplicate inline checks for consecutive '$' characters in
parser.cpp can be replaced by a small helper to improve readability; add a
helper like int dollarRun(const char *p) or bool dollarRun(const char *p, int n)
and use it in the branches that currently use chains like '$' == p[0] && '$' ==
p[1] ... (the branches that call GetLabelPhPage and GetLabelPhValue and the
existing DISP_NONE != PseudoORG branch), then update those conditionals to call
the helper while preserving the existing isLabelStart(p+3)/isLabelStart(p+4)
checks and short-circuit/NUL-safety so behavior remains identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 283c1201-b8a4-401d-a266-7a6977cfc5f7
📒 Files selected for processing (5)
docs/documentation.xmlsjasm/parser.cppsjasm/tables.cppsjasm/tables.htests/devices/get_label_physical_memory_operators.asm
…lose #328 Since internally these are tracked for SIZEOF implementation, it's reasonably easy to surface them to user as cross-over with already existing $$$ and $$$$ operators. The label versions are not confined to device mode, but also the logical results come for regular labels only (for non-DISP the physical address and page is equal to regular address and page of course, inside DISP block the physical values show where the machine code landed). For EQU/DEFL/SMC-offset labels the results are less designed and more "what happens as implementation side-effect", but they are documented by adding them to the test, so the user can actually depend on this "logic" in their own source, in case it is practical for anything. The only really "unfortunate" behavior is struct-member-label which has physical address of the whole structure, not the member field, this is noted in test as "unfortunate" and workaround for more logical physical address of member expression suggested. But I think I will not ever change even that, doesn't seem to be worth the extra complexity of implementation for feature which has probably no real world use case, and can be easily worked around if really needed.
ped7g
force-pushed
the
labels_ph_features_328
branch
from
d0f2537 to
7b395ff
Compare
1 participant
Since internally these are tracked for SIZEOF implementation, it's reasonably easy to surface them to user as cross-over with already existing $$$ and $$$$ operators.
The label versions are not confined to device mode, but also the logical results come for regular labels only (for non-DISP the physical address and page is equal to regular address and page of course, inside DISP block the physical values show where the machine code landed).
For EQU/DEFL/SMC-offset labels the results are less designed and more "what happens as implementation side-effect", but they are documented by adding them to the test, so the user can actually depend on this "logic" in their own source, in case it is practical for anything.
The only really "unfortunate" behavior is struct-member-label which has physical address of the whole structure, not the member field, this is noted in test as "unfortunate" and workaround for more logical physical address of member expression suggested.
But I think I will not ever change even that, doesn't seem to be worth the extra complexity of implementation for feature which has probably no real world use case, and can be easily worked around if really needed.
Summary by CodeRabbit
New Features
$$$labeland$$$$labeloperators to query physical address and page information for any label, complementing existing physical program counter operators.Documentation