Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: move string literals to constants #4477

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

leongross
Copy link
Contributor

No description provided.

@aykevl
Copy link
Member

aykevl commented Sep 19, 2024

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.

Copy link
Contributor Author

@leongross leongross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo removing the string constants has several benefits. First, it removes the possibility for typos and inconsistencies. A lot of these strings are used in several places with the same value. If the names are descriptive enough I think it eases the development workflow and prohibits typos, also it makes the relations clearer (i.e. BinFormat{...}) lists all the available BinFormats, so no need to look up all the possible definitions in the code. I agree, putting this into an enum is also a good idea. But since many of these options are passed literally to llvm and other build steps (I think) it makes sense to keep this as a string.

@aykevl
Copy link
Member

aykevl commented Sep 21, 2024

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).

Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants