Use enum (flags) instead of hardcoded string values for emit kinds #6515
Conversation
src/compiler/crystal/compiler.cr
Outdated
| @@ -98,13 +98,21 @@ module Crystal | |||
| # and can later be used to generate API docs. | |||
| property? wants_doc = false | |||
|
|
|||
| # Can be set to an array of strings to emit other files other | |||
| @[Flags] | |||
| enum EmitKinds | |||
There was a problem hiding this comment.
Better have the name in singular, that's used throughout the code base.
Maybe EmitTarget would even be a better name.
There was a problem hiding this comment.
And I'd suggest to have the values either all uppercase or all lower case. I prefer the former.
src/compiler/crystal/command.cr
Outdated
| @@ -309,7 +307,10 @@ class Crystal::Command | |||
| end | |||
|
|
|||
| unless no_codegen | |||
| opts.on("--emit [#{VALID_EMIT_VALUES.join('|')}]", "Comma separated list of types of output for the compiler to emit") do |emit_values| | |||
| valid_emit_values = {{ Compiler::EmitKinds.constants.map(&.stringify).reject { |m| %w(none all).includes?(m.downcase) } }} | |||
There was a problem hiding this comment.
Why use a macro instead of Enum.names?
There was a problem hiding this comment.
Wow how did I miss that (PR-ing at 3am may be why 😄 )
thanks!
src/compiler/crystal/compiler.cr
Outdated
| when "obj" | ||
| FileUtils.cp(object_name, "#{output_filename}.o") | ||
| def emit(emit_kinds : EmitKinds, output_filename) | ||
| emit_kinds.each do |kind| |
There was a problem hiding this comment.
Why use a loop? You can just have consecutive conditionals:
if emit_kinds.asm?
# do asm
end
if emit_kinds.llvm_bc?
# do llvm-bc
end
# etc...There was a problem hiding this comment.
I was just matching the previous behavior, but that's right, it makes no sense to have a loop now
|
Thank you, but I disagree. The code became much harder to write, read and understand. |
|
31 additions and 23 deletions. Now we have more code. Also more macro code, which is always harder to understand. A gsub/tr call to replace dash with underscore. The old code was just fine. |
|
@asterite How is it much harder to write/read/understand?
The macro code has been removed as I didn't noticed
I don't like it either, that's why I asked what I should do in the OP.
For the current use-case I agree, but I'd like to change when some emit targets are emitted (for #5820 & #5821), and I really don't want to have hardcoded strings related to CLI input lying around when an enum flag can do the trick (and be typesafe!). |
src/compiler/crystal/command.cr
Outdated
| @@ -309,7 +307,10 @@ class Crystal::Command | |||
| end | |||
|
|
|||
| unless no_codegen | |||
| opts.on("--emit [#{VALID_EMIT_VALUES.join('|')}]", "Comma separated list of types of output for the compiler to emit") do |emit_values| | |||
| valid_emit_values = Compiler::EmitTarget.names | |||
| valid_emit_values.map! { |v| v.tr("_", "-").downcase } | |||
There was a problem hiding this comment.
You should use Chars instead of Strings here.
ditto below.
There was a problem hiding this comment.
Yeah that's what I did originally, but there is only tr(String, String) not tr(Char, Char)
It has just been moved into Under the current circumstances, @asterite is probably right that this isn't really useful as an improvement. But in the light of changes to the locations where these flags are used, it's a different story. |
|
The CLI options could just hard coded into the description string to avoid the macro. It's not that they change all the time to be automatically generated. |
I think that using enum flags is cleaner in this case.
It's better to remove handcoded emit values from obscure places in the compiler code 😃
I tried to keep the emit values the same (e.g:
llvm-ir), but it would be simpler (?) to usellvm_iras it's how the enum flag can be easily parsed. This could be changed though, but a breaking change.This is the beginning to work on #5820, and maybe #5821.