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

Restrict CCompRC::LoadResourceFile and usage to win #41757

Merged
merged 1 commit into from
Sep 3, 2020
Merged

Restrict CCompRC::LoadResourceFile and usage to win #41757

merged 1 commit into from
Sep 3, 2020

Conversation

am11
Copy link
Member

@am11 am11 commented Sep 2, 2020

String resouces packaged as PE files only exist on Windows.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas
Copy link
Member

jkotas commented Sep 2, 2020

It is not interesting to do anything here unless it is part of a larger plan for localization of error messages produced by unmanaged code. You can replace the UNIXTODO with a comment or something.

cc @janvorli

@am11 am11 closed this Sep 3, 2020
@am11 am11 reopened this Sep 3, 2020
@am11 am11 changed the title Implement CCompRC::LoadResourceFile for Unix Restrict CCompRC::LoadResourceFile and usage to win Sep 3, 2020
@am11 am11 marked this pull request as ready for review September 3, 2020 12:00
@@ -605,6 +609,8 @@ HRESULT CCompRC::LoadLibraryHelper(HRESOURCEDLL *pHInst,
return hr;
EX_TRY
{
// String resouces packaged as PE files only exist on Windows
#ifdef HOST_WINDOWS
Copy link
Member

Choose a reason for hiding this comment

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

You can use just single #ifdef / #endif with th3 #ifdef before the EX_TRY above and #endif before the return.

Copy link
Member

Choose a reason for hiding this comment

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

The whole function should be unreachable on non-Windows. It would be best to itdef-out the whole function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved LoadLibraryHelper and LoadResourceFile functions under a single ifdef.

String resouces packaged as PE files only exist on Windows.
@jkotas
Copy link
Member

jkotas commented Sep 3, 2020

Test failure is #37132

@jkotas
Copy link
Member

jkotas commented Sep 3, 2020

Thanks!

@jkotas jkotas merged commit ed94902 into dotnet:master Sep 3, 2020
@am11 am11 deleted the feature/unixtodo/LoadResourceFile branch September 3, 2020 22:30
@ghost ghost locked as resolved and limited conversation to collaborators Dec 28, 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.

4 participants