Add named parameters for macros. #339
Conversation
…instead RECT x=1,y=2,dx=3,dy=4
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. 📝 WalkthroughWalkthroughThis PR adds named macro argument support to sjasmplus. The assembler's ChangesNamed macro arguments feature
Sequence DiagramsequenceDiagram
participant Parser as Macro Parser
participant Emit as CMacroTable::Emit
participant Validator as Named Arg Validator
participant Storage as Argument Map
Parser->>Emit: Invoke macro with argname = value
Emit->>Emit: Detect named syntax?
alt Named syntax detected
Emit->>Validator: Parse each argname = value
Validator->>Storage: Is name in parameter list?
alt Valid name
Storage->>Storage: Track seen names
alt Not duplicate
Storage->>Storage: Store argument value
else Duplicate
Storage-->>Emit: Error: duplicate argument
end
else Invalid name
Storage-->>Emit: Error: invalid name
end
Emit->>Validator: All required arguments provided?
alt Yes
Validator-->>Emit: Validation complete
else No
Validator-->>Emit: Error: missing arguments
end
else Positional syntax
Emit->>Emit: Use legacy positional parsing
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sjasm/tables.cpp`:
- Around line 986-989: When parsing macro arguments you currently restore
macrolabp on error but leave entries added by AddMacro in MacroDefineTable; save
a snapshot of MacroDefineTable (e.g., its size or iterator/marker) before
starting to parse arguments and on every error-return path (the Error(...) then
macrolabp = omacrolabp; return 1; paths around AddMacro) restore
MacroDefineTable to that snapshot before returning; update all similar error
blocks (the blocks at the shown snippet and the ones around the identifiers
macrolabp, omacrolabp, AddMacro, MacroDefineTable at the other mentioned ranges)
to perform the same restore so no partial argument defines leak.
🪄 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: 2c2922cf-19dd-4a06-92ea-42ec8533e983
📒 Files selected for processing (3)
sjasm/tables.cpptests/macros/named_macro_args.asmtests/macros/named_macro_args.lst
ped7g
left a comment
ped7g
left a comment
There was a problem hiding this comment.
PR noted, but please ton of patience.
a) migration of CI is not fully finished and I will ping you back and request rebase after it is
b) I was hoping to finalize attempt to do .deb packaging before any bigger change (then enforced CI migration happened), so I may prefer to land this after .deb (or before ... depends how things will go with either thing).
c) meanwhile any attempt at updating documentation.xml would be nice addition to this PR
d) this theoretically breaks old valid syntax like db_macro x=$. But I don't believe anyone is using such macro argument value in live project, so I'm ok with changing syntax rules to "needs <> when content has = char", but should be documented and the delimited content inside <> must work of course (seems it does, although you have also quotes there in the test, so it's not 100% clear).
Also please review: #72 #103 #150
and consider those ideas vs your change, if it happens to collide with anything and how, I think none of those should be affected by this, but reviewing it and being sure is better than just wild guess.
The syntax/rules should be clear and agreed before I would even take a look at the code itself, not reviewing that yet (plus pending CI and .deb issues being my current focus).
(obviously other maintainer of sj+ may step in at any time, but right now I'm only active one, so I'm a bit in a position of benevolent dictator, so have patience with me :D )
| DB val | ||
| ENDM | ||
|
|
||
| ONE val=10 |
There was a problem hiding this comment.
try also:
val: ONE <val=$>
this should pass with value val=$ ie. DB val=$ ie. DB -1 -> 0xFF.
And not sure what the valid syntax should be for named variant:
ONE <val=val=$> ; everything is content, no named annotation here
ONE val=<val=$> ; I guess this should work as `DB val=$`
I guess this one should pass as valid, val= is just name pinning annotation and <...> is valid delimited argument content.
Weird one:
ONE val=val=$
Not sure about this one, technically after removing name of arg rest val=$ is content, so if your solution will accept this and end up with DB val=$, I'm ok with it. But if you will report this as error and require delimited content in <>, it feels consistent as some kind of rule ("to have = in argument content, use delimited way")
In any case, please document these in test how they work or not, so we can discuss particular edge cases, what already works and what may be worth of change.
There was a problem hiding this comment.
What do you think about using := for named variables?
LOAD_XY x:=1, y:=2
I believe this will keep compatibility with the usage of = as the equal operator.
(I will refrain from updating the PR until we have a good syntax.)
There was a problem hiding this comment.
No, := has two issues:
-
:splits lines (well before parser, at stream reading level, so the whole logic of detecting you are trying to call macro and not splitting line would have to move somewhere where it does not belong. -
no matter what you chose for the syntax, it will always collide with old syntax, because in previous versions whatever-you-picked is sent as argument content.
ie. you chose for example @@@=@@@ and do ONE val@@@=@@@10 and the old sjasmplus will start executing body of macro ONE with val content equal to val@@@=@@@10.
The new version will then have val content equal 10.
Whether you may have ever needed @@@=@@@ in old source is not that important, because the need of = is already highly unlikely, probably nobody ever wrote that as macro input.
So let's accept there is clash and this is "breaking" change and rather focus to make it convenient to use and to not blatantly block possible future extensions (mentioned extra issues with ideas about macro arguments).
So x=123 syntax is OK for me, I don't think you can do better.
There was a problem hiding this comment.
try also:
val: ONE <val=$>ONE <val=val=$> ; everything is content, no named annotation here
On the current version, before the named params patch, both these cases lead to infinite recursion (eventually blocked by 31-limit or line too long).
I would propose that using the name of the param as a token when calling the macro should be an error. So your four ONE examples should be errors, because you are passing "val" and in macro with param name "val". Do you agree with this? Otherwise we would need to solve the infinite recursion problem before.
There was a problem hiding this comment.
Oh, I forgot about that, that's excellent news, so there's zero chance of val= as content in legacy sources, because it would cause error in assembling. So actually your initial suggestion doesn't even clash with old syntax (by side-effect of substitution rules causing infinite loop).
I think this is then OK as is, ie. ONE val=$ will AFTER your change works because it's named argument, and ONE <val=$> will end in infinite recursion as before (confirm by test). The infinite recursion is good-enough error, no need for specific detection of name of param, the substitution detects it by loop. :)
So I would just add this to test to showcase the current behavior (inf. loop). If I did misunderstood something again, I will understand it from test, so that's the best way to tell me what happens. :)
|
|
||
| LOAD_XY x=1, z=2 ; unknown argument name | ||
| LOAD_XY x=1, x=2 ; duplicate argument name | ||
| LOAD_XY x=1 ; missing argument 'y' |
There was a problem hiding this comment.
what about LOAD_XY x=1, 2 and LOAD_XY 2,x=1?
I think in some languages once you use one name annotation, you are required to use all of them, in other languages the first would do implicit y=2, I hope the second variant doesn't work anywhere, because that's cursed.
What is your opinion + document it in test how it works for these cases where amount of args is correct, but explicit/implicit naming is mixed (I don't have preference right now, I would go with simpler implementation either supporting mixed way or error out, requiring all of them named is maybe slightly more "consistent" and less error prone, but implementation must be sane).
There was a problem hiding this comment.
Both cases should be errors, I think. Once one parameter is named, all should be named.
|
Regarding the open issues: #72 #103 #150 |
|
Ad CI migration: ok, I hope I'm done with it. If you would patch the test to reflect the discussion and rebase on current master, it would be nice. |
2 participants
Macros with lots of parameters can often become illegible.
For example, in "RECT 1, 2, 3, 4", what is the meaning of the numbers?
Instead of checking the definition of the macro, you can use named parameters to self-document the intent: "RECT x=1,y=2,dx=3,dy=4".
This PR implements the named parameters and add tests for it.
Summary by CodeRabbit
New Features
argname = value), allowing arguments to be supplied in any order regardless of declaration orderTests