Conversation
pfusik
left a comment
pfusik
left a comment
There was a problem hiding this comment.
We don't have to provide long aliases to all the short options right away. The long options should serve future expansions not 1:1 mapping of short options.
The obvious ones are listing=on, listing=off, cpu=65c02, cpu=6502.
| break; | ||
| case "HARDWARE": | ||
| switch (optionValue(name, hasValue, value)) { | ||
| case "A800": |
There was a problem hiding this comment.
| case "A800": | |
| case "ATARI800": |
| case "A800": | ||
| optionHardware = Hardware.a800; | ||
| break; | ||
| case "A5200": |
There was a problem hiding this comment.
| case "A5200": | |
| case "ATARI5200": |
|
|
||
| void setNamedOption(string name, bool hasValue, string value) { | ||
| switch (name) { | ||
| case "FILL": |
There was a problem hiding this comment.
How about combining three short options into one long:
opt o- = opt 'output=off'
opt o+h-f- = opt 'output=raw'
opt o+h-f+ = opt 'output=rom'
opt o+h+f- = opt 'output=ataridos' ; default
opt o+h+f+ ; not useful, no long alias
There was a problem hiding this comment.
Looks good. I'd add a few restrictions:
- setting
outputis an error after anything has been emitted to object, - setting
outputtwice is an error, - setting
o,h, orfafteroutputhas been set explicitly is an error.
Without output, the single-letter options o, h and f work as previously.
There was a problem hiding this comment.
Maybe the above are too restrictive – one may want to get label table and/or listing without the object bytes, and opt 'output=spartadosx'; opt o- would be a quite self-descriptive way to spell it, even if not the most elegant.
Currently o- is allowed even mid-block, which I consider a bug because it generates broken binaries. Not sure if there's any code out there that depends on it.
I'd disallow opt o- completely after the first object byte is emitted, unless there is a good reason to keep it allowed between blocks.
There was a problem hiding this comment.
It may also make sense to get o out of output (so the latter would only set h and f), and provide 'object=on|off' so that it can be set in one opt with output.
There was a problem hiding this comment.
Ignore the last two comments. I don't think there's a use case for o- after output=....
| case "OBJECT": | ||
| optionObject = optionBool(name, hasValue, value); | ||
| break; | ||
| case "UNUSED-LABELS": |
There was a problem hiding this comment.
Not bad, but:
unusedalone seems a bit confusing, especially if at some point we find out that it's useful to warn about unused something else.- How do we toggle various warnings independently?
There was a problem hiding this comment.
Example:
opt 'warn_unused_labels=on,warn_cross_page_branch=off' – it's clear which warnings are toggled, and that all others aren't.
opt 'warn=unused_labels' – does it only enable warning on unused labels? Or does it enable this one and disable everything else? How to disable the warning? no_warn=unused_labels? warn=no_unused_labels? Unintuitive.
|
|
||
| bool optionBool(string name, bool hasValue, string value) { | ||
| if (!hasValue) | ||
| return true; |
There was a problem hiding this comment.
I don't feel this syntax sugar is any good.
| assert(0); | ||
| } | ||
|
|
||
| string readOptionName() { |
There was a problem hiding this comment.
Can we extract [0-9A-Za-z_?]+ lexing from readLabel, returning an uppercased string, and reuse it for option names and values?
2 participants
Prerequisite for stuff like
opt 'cpu=65c02'(#16) oropt 'format=sdx'(#17).