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

c#: Mechanical refactors #1110

Merged
merged 6 commits into from
Dec 20, 2024

Conversation

jsturtevant
Copy link
Collaborator

There are a lot of changes here but no functional changes. Only a couple renames and mostly moved structs to their own modules to help with code organization and make it easier for some one new to understand the code. There are probably a lot of little improvements that could be made after this in an incremental way to reduce sharing across the modules but this is a good start. Each move/rename was done in a separate commit with cargo fmt/cargo check and some tests run against it.

I am suggesting these changes after having spent some time in the code. It took my awhile to understand the general flow and I think having the code organized in this way will help others understand the code faster.


impl ToCSharpIdent for str {
// Source: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/
fn csharp_keywords() -> Vec<&'static str> {
Copy link
Collaborator

@dicej dicej Dec 19, 2024

Choose a reason for hiding this comment

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

It would be more efficient to make this return a &'static [&'static str] instead of allocating and populating a fresh new Vec every time it's called. Something like:

fn csharp_keywords() -> &'static [&'static str] {
    static ARRAY: &[&str] = &["abstract", ...];
    ARRAY
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since that requires a bit more refactoring than just moving. I'll follow up on it with another PR

@jsturtevant jsturtevant added this pull request to the merge queue Dec 20, 2024
Merged via the queue into bytecodealliance:main with commit 9b91987 Dec 20, 2024
25 checks passed
@jsturtevant jsturtevant deleted the mechanical-refactors branch December 20, 2024 18:18
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