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

Make H3StaticTable class static #36222

Merged
merged 4 commits into from
May 29, 2020
Merged

Make H3StaticTable class static #36222

merged 4 commits into from
May 29, 2020

Conversation

Youssef1313
Copy link
Member

No description provided.

@ghost
Copy link

ghost commented May 11, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

Comment on lines -42 to -50
private H3StaticTable()
{
}

public static H3StaticTable Instance { get; } = new H3StaticTable();

public int Count => _staticTable.Length;

public HeaderField this[int index] => _staticTable[index];
Copy link
Member Author

Choose a reason for hiding this comment

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

The indexers aren't allowed in static class. I tried to check if it was used any where but found none. However, I can't confirm because I run into build problems on my machine, so I'll wait for the CI build to finish and check.

@Tratcher Tratcher requested a review from jkotalik May 11, 2020 17:04
@Tratcher
Copy link
Member

@Youssef1313
Copy link
Member Author

@karelz karelz requested a review from scalablecory May 11, 2020 17:48
@Tratcher
Copy link
Member

Isn't this done automatically, just like dotnet/aspnetcore#20640?

That's a fail-safe to make sure we don't miss anything. We want to know changes compile and work in both repos before merging them in either. That said, this one's probably OK since it didn't change any of the API surface area consumed by HttpClient or Kestrel. We can auto-merge it when the tests pass.

@Youssef1313
Copy link
Member Author

Isn't this done automatically, just like dotnet/aspnetcore#20640?

That's a fail-safe to make sure we don't miss anything. We want to know changes compile and work in both repos before merging them in either. That said, this one's probably OK since it didn't change any of the API surface area consumed by HttpClient or Kestrel. We can auto-merge it when the tests pass.

Ok, I'm going to create a similar PR in aspnetcore

@Youssef1313
Copy link
Member Author

After giving a look, it won't actually compile correctly in aspnetcore 😆
The indexer wasn't used here, but was used there. That's the reason it existed.

Because indexers can't be used in static class, I'll have to make a method instead of the indexer. Should this method be included here also? (given that it's used only in aspnetnet repo but not here)

@Tratcher
Copy link
Member

Should this method be included here also?

Yes, the shared files should match exactly. There's instructions in the readme for copying the files. You can develop the fix in either repo, but make sure to copy it when you're done.

@Youssef1313
Copy link
Member Author

Should this method be included here also?

Yes, the shared files should match exactly. There's instructions in the readme for copying the files. You can develop the fix in either repo, but make sure to copy it when you're done.

Thanks for guidance, I'll fix it soon.

@Youssef1313
Copy link
Member Author

I opened dotnet/aspnetcore#21705

@@ -28,7 +27,7 @@ internal class H3StaticTable
[500] = 71,
};

private readonly Dictionary<HttpMethod, int> _methodIndex = new Dictionary<HttpMethod, int>
private static readonly Dictionary<HttpMethod, int> _methodIndex = new Dictionary<HttpMethod, int>
Copy link
Member

@stephentoub stephentoub May 12, 2020

Choose a reason for hiding this comment

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

All of these static fields should now be renamed to have an s_ rather than just _ prefix.

@Youssef1313
Copy link
Member Author

@Tratcher @stephentoub The parallel PR (dotnet/aspnetcore#21705) is merged.

@stephentoub stephentoub changed the title Make class static Make H3StaticTable class static May 29, 2020
@ghost
Copy link

ghost commented May 29, 2020

Hello @Tratcher!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@stephentoub
Copy link
Member

@Tratcher, should I merge this? Not sure how the mirror works.

@Tratcher
Copy link
Member

@stephentoub yes please. Ideally we merge the changes into both repos on the same day and then the mirror doesn't have to do anything. It's only there as a failsafe.

@stephentoub stephentoub merged commit fec84ae into dotnet:master May 29, 2020
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants