tinygo-org / tinygo

Go compiler for small places. Microcontrollers, WebAssembly (WASM/WASI), and command-line tools. Based on LLVM.
https://tinygo.org
Other
15.48k stars 913 forks source link

ref: move string literals to constants #4477

Open leongross opened 2 months ago

aykevl commented 2 months ago

What's the goal here? Is the idea that this improves readability? I find most of these constants to be harder to read to be honest. It might be useful in some cases (for example, for GC values, but then why not use an enum with numbers instead?), but in most cases I think the extra indirection only makes it harder to read/understand the code.

aykevl commented 1 month ago

First, it removes the possibility for typos and inconsistencies.

True, but does that actually happen? Did you find any bugs with these issues? While I am normally in favor of moving things into constants, I'm not entirely convinced the constants in this PR make things easier to read (remember, code is read far more often than it is written). And I also think most of such typos will be caught very quickly.

There are some cases where I think constants could be helpful, but those are in fact enums. For example for GC or scheduler types. They could be used like this, for example:

type GCType string

const (
    GCConservative GCType = "conservative"
    GCPrecise      GCType = "precise"
    // etc
}

(Also, I think you forgot to add some new files in this PR).

leongross commented 1 month ago

There are some cases where I think constants could be helpful, but those are in fact enums. For example for GC or scheduler types. [...]

I agree making changing the const strings to enums is a good idea. But I am not sure I fully understand which cases you see as useful and which not.