Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Remove ToString from RuntimeAssembly.GetManifestResourceStream#22012

Merged
stephentoub merged 2 commits into
dotnet:masterfrom
stephentoub:runtimeassemblycharstring
Jan 17, 2019
Merged

Remove ToString from RuntimeAssembly.GetManifestResourceStream#22012
stephentoub merged 2 commits into
dotnet:masterfrom
stephentoub:runtimeassemblycharstring

Conversation

@stephentoub
Copy link
Copy Markdown
Member

A small allocation we can avoid with the span-based string.Concat.

A small allocation we can avoid with the span-based string.Concat.
@filipnavara
Copy link
Copy Markdown
Member

There's one more instance of the same code in ManifestBasedResourceGroveler. I have a refactoring PR in queue to merge them (#21979).

string delimiter = (nameSpace != null && name != null) ? Type.Delimiter.ToString() : null;
string resourceName = string.Concat(nameSpace, delimiter, name);

char c = Type.Delimiter;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you feel like it you could change nameSpace to namespace ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not without putting an @ in front of it, as it's a reserved keyword.


char c = Type.Delimiter;
string resourceName = nameSpace != null && name != null ?
string.Concat(nameSpace, new Span<char>(ref c, 1), name) :
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ReadOnlySpan to avoid Span->ReadOnlySpan cast?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup. I'm pretty sure I've made this mistake in other places, too; I'll do a pass to clean them up.

@stephentoub
Copy link
Copy Markdown
Member Author

There's one more instance of the same code in ManifestBasedResourceGroveler. I have a refactoring PR in queue to merge them (#21979).

Thanks, @filipnavara. If I merge this, will that mess up your work?

@filipnavara
Copy link
Copy Markdown
Member

If I merge this, will that mess up your work?

It's fine, I will rebase the other PR.

@stephentoub
Copy link
Copy Markdown
Member Author

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests please

@stephentoub
Copy link
Copy Markdown
Member Author

It's fine, I will rebase the other PR.

Ok, thanks!

@stephentoub stephentoub merged commit b9788c2 into dotnet:master Jan 17, 2019
@stephentoub stephentoub deleted the runtimeassemblycharstring branch January 17, 2019 23:10
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…t/coreclr#22012)

* Remove ToString from RuntimeAssembly.GetManifestResourceStream

A small allocation we can avoid with the span-based string.Concat.

* Address PR feedback


Commit migrated from dotnet/coreclr@b9788c2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants