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

Conversation

@nxtn
Copy link

@nxtn nxtn commented Apr 10, 2019

@jkotas commented 2 years ago in https://github.com/dotnet/coreclr/issues/12266#issuecomment-308484006

Yes, the dlclose call should be removed.

Is that still true?

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit 448e39d into dotnet:master Apr 10, 2019
@nxtn nxtn deleted the patch-2 branch April 10, 2019 04:19
@nxtn
Copy link
Author

nxtn commented Apr 10, 2019

Completely unrelated to this PR, you shouldn't use three different naming of CoreCLR, even in one file.

void* coreclrLib = dlopen(coreClrDllPath.c_str(), RTLD_NOW | RTLD_LOCAL);
if (coreclrLib != nullptr)
{
coreclr_initialize_ptr initializeCoreCLR = (coreclr_initialize_ptr)dlsym(coreclrLib, "coreclr_initialize");

Could you standardize the naming? I suggest using CoreClr or coreClr, except for the Unix style APIs.

@jkotas
Copy link
Member

jkotas commented Apr 10, 2019

I believe that the two most widely used forms are CoreCLR and coreclr. If you would like to submit a PR to clean it up, I would recommend to stick with these two. Also, the coreclr prefixes and suffixes in this file are not always helpful - they can be just removed in some cases (e.g. when they are used for names of locals).

@nxtn
Copy link
Author

nxtn commented Apr 10, 2019

Should these calls to FreeLibrary also be removed?

::FreeLibrary(m_coreCLRModule);

::FreeLibrary(m_coreCLRModule);

@jkotas
Copy link
Member

jkotas commented Apr 10, 2019

Yes, these can be deleted as well.

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.

2 participants