introduction of macroArg command #1481
Conversation
Macro args declarations
attempt at macroArg parser
improved error messages
sync with origin
sync to origin
…eyboard/fix/gperf-arg-expansion Fix gperf token vs argument expansion bug.
resync with origin
sync from origin
…firmware into macroArgs-declarations
…ue the processCommand() loop
too memory intensive implementation of macro arguments storage, needs rework
right/src/macros/core.h
Outdated
| bool autoRepeatInitialDelayPassed: 1; | ||
| macro_autorepeat_state_t autoRepeatPhase: 1; | ||
| // ---- 4-aligned ---- | ||
| macro_arg_t arguments[MAX_MACRO_ARGUMENT_SIZE]; |
There was a problem hiding this comment.
No. Definitely not allowing this into origin.
As discussed, these should be allocated from a shared pool on demand.
There was a problem hiding this comment.
Yes, understood. I'm not even filling the structure out currently. I will remove it for now until I have a better storage.
right/src/str_utils.c
Outdated
| consumeWhite(ctx); | ||
| } | ||
|
|
||
| void ConsumeIdentifier(parser_context_t* ctx) |
There was a problem hiding this comment.
😱
No. Don't publish this very unsafe thing as an Api.
Also, it is never used.
There was a problem hiding this comment.
Do all your Consume... interfaces move past White, or why do you consider this unsafe?
You are right, it's not being used. Must be a leftover from an earlier attempt. Will remove it.
There was a problem hiding this comment.
Whites now handle template expansions.
I am generally trying to keep context handling contained here in str_utils and isolate higher layers from having to deal with separators, whites, and generally tokenization. This api, including the ":" syntax, goes against this effort.
As I said, I strongly favor a 'macroArg int name "string literal"' format. (I am generally fond of the scala/kotlin 'name: type' style, but I feel we are here in a c-standard domain and so should stick to its conventions of 'type name'.)
There was a problem hiding this comment.
I do not want "type name". That's old school. Modern languages use "name first, followed by type" order.
My view is that the types are additional, optional properties of the parameters. The ':' notation is more towards the style of e.g. Rust. It is optional; macroArg the_param My beautiful param will map as any type.
Readability: all the names are at the same position, if you have the type first the indentation will vary.
macroArg this_argument:int Number of customers
macroArg the_other_argument:string Country name
Dynamic typing or type inference: leave out the optional :type for an any type.
Type theory: Type is a property of a value, name-first mimics this.
My stream of thoughts:
macroArg x:int Initial counter
- This macro has an argument
- It is called 'x'
- btw, it is an integer
- Fill it out with the initial counter value
We may need to agree to disagree here.
There was a problem hiding this comment.
Alright.
But please really enclose those comments in quotes:
macroArg this_argument:int "Number of customers"
macroArg the_other_argument:string "Country name"
Unquoted strings are allowed only for backward compatibility.
There was a problem hiding this comment.
OK, I will make the description a proper string.
There was a problem hiding this comment.
OK, I will make the description a proper string.
right/src/macros/commands.c
Outdated
| } \ | ||
| break; | ||
|
|
||
| static macro_result_t dispatchCommand(parser_context_t* ctx, command_id_t commandId, bool *headersFinished) { |
There was a problem hiding this comment.
Could we maybe leave the switch where it was?
At the very minimum in order to make the diffs readable for now.
There was a problem hiding this comment.
I really wanted to pull out the long switch because it makes the logic in processCommand() more compact and obvious. I want to express the core of the command processing while loop, and which results trigger which next activities (some loop, some don't). I would really like to leave it in dispatchCommand(). It's a one-time diff, then it'll stay where it is.
There was a problem hiding this comment.
Having this diff live on a long lived feature branch (that this is probably going to be) makes it unsafe - it will tend to create conflicts that are not easily detectable and resolvable.
If we are to do this refactor (I still don't see it as important or necessary), then we need to do it via a dedicated PR.
There was a problem hiding this comment.
I can create a mini-refactor just for this, if that's your preference. I understand you don't want this block of change to live for weeks or even months of diffs; that makes sense.
Macro args declarations, refactor
2 participants
Draft for @kareltucek to review