More cleanups in Assembly/Loader/Binder area #58462
Conversation
|
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov Issue Details
|
…extBinder -> FallbackBinder, etc ...
src/coreclr/vm/appdomain.cpp
Outdated
| hrBindResult = bindResult.GetHRBindResult(); | ||
|
|
||
| if (bindResult.Found()) | ||
| if (bindResult) |
There was a problem hiding this comment.
Nit: I would rename the local variable to something like boundAssembly - basically include the word "Assembly" in it. It feels weird to read the code below and use a "result" as an assembly. (Not that it was better before, it was even worse)
There was a problem hiding this comment.
Right. I actually thought that boundAssembly could be a better name here.
I think I will make another pass over variables typed as AssemblyBinder*. We have a variety of [ load | context | binder ] combinations when naming these. Perhaps calling them binder or pBinder, in majority of cases at least, would make it more consistent.
src/coreclr/vm/assembly.cpp
Outdated
| { | ||
| pAssemblyLoaderAllocator->EnsureReference(pBinderAssemblyLoaderAllocator); | ||
| pAssemblyLoaderAllocator->EnsureReference(pAssemblyLoaderAllocator); |
There was a problem hiding this comment.
This change is a bug. The pBinderAssemblyLoaderAllocator needs to stay or to be renamed to something else than pAssemblyLoaderAllocator. The motivation for the name was that it is the loader allocator that came from the binder and we need to distinguish it from the other one.
There was a problem hiding this comment.
oh, right, thee are two of them here. Thanks!
| @@ -30,6 +30,11 @@ class DefaultAssemblyBinder final : public AssemblyLoadContext | |||
| return NULL; | |||
| } | |||
|
|
|||
| bool IsDefault() | |||
There was a problem hiding this comment.
I would prefer this to be marked as virtual (or override) to make it obvious that it is an override of the base method. The same at the other places.
src/coreclr/vm/assemblynative.cpp
Outdated
|
|
||
| if (assemblyLoadContext.Get() != NULL) | ||
| { | ||
| INT_PTR nativeAssemblyLoadContext = ((ASSEMBLYLOADCONTEXTREF)assemblyLoadContext.Get())->GetNativeAssemblyLoadContext(); | ||
| pBinderContext = reinterpret_cast<AssemblyBinder*>(nativeAssemblyLoadContext); | ||
| INT_PTR nativeAssemblyLoadContext = ((ASSEMBLYLOADCONTEXTREF)assemblyLoadContext.Get())->GetNativeAssemblyBinder(); |
There was a problem hiding this comment.
A nit - maybe rename the local to nativeAssemblyBinder or something like that?
There was a problem hiding this comment.
Yes, nativeAssemblyBinder will be better.
|
Thanks!!! |
ghost
locked as resolved and limited conversation to collaborators
AssemblyLoadContextinto its baseAssemblyBinderNow
DefaultAssemblyBinderandCustomAssemblyBinderderive directly fromAssemblyBinderTPABinderContext -> DefaultBinder, FallbackLoadContextBinder -> FallbackBinder, etc ...
CoreBindResultand everything undersrc/coreclr/vm/coreclrfolderThe type is basically a tuple of
BINDER_SPACE::AssemblyandHRESULTandHRESULTis used in a single place right after it is set. The rest is COM support, ref-counting, holder, init/reset/delete, and other plumbing. We can just useAssemblydirectly.