search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.19k stars 4.72k forks source link

Consider renaming COM* types in clr native code to CLR* #86145

Open huoyaoyuan opened 1 year ago

huoyaoyuan commented 1 year ago

Currently, there are still some native code elements named with COM prefix from the COM+ era, like COMFloat and COMDouble:

https://github.com/dotnet/runtime/blob/a5d13e24cb374b55c906061f825d864efec7f3fb/src/coreclr/classlibnative/inc/floatdouble.h#L10

Can we rename them to things like CLRDouble, to make a clear distinction with COM interop? Are there any tools depending on these names?

AaronRobinsonMSFT commented 1 year ago

Are there any tools depending on these names?

There is no reason these can't be changed. Good suggestion.

AaronRobinsonMSFT commented 1 year ago

If someone wants to take this up, it would be most appropriate to provide a list of the types and APIs that will be updated.

jkotas commented 1 year ago

Most of these COM* types are holding types for FCalls. The newer naming convention is to use Native suffix. For example, AssemblyNative, MarshalNative, ObjectNative, StreamNative, ... .

Also, we may want to change FCalls to be extern "C" methods, similar to what we have done with QCalls a while back in #60992. It would set us up for convergence with native AOT. FCalls in native AOT have to be extern "C" methods to enable linking managed code with unmanaged runtime using platform linker.

huoyaoyuan commented 1 year ago

A list of symbols that can be renamed at my site of view:

COM interop related: ComPlusCall -> ClrToComCall (the file has been renamed) ComPlusCallInfo ComPlusCallMethodDesc etc. Remove the "ComPlus" name.

FCall related: COMDateTime COMDelegate comdependenthandle.h comdynamic.h (QCall) COMModule COMSynchronizable.h comthreadpool.h comwaithandle.h COMCustomAttribute COMString gCOM*Funcs in ecalllist.h COMDouble COMSingle COMVariant COMOAVariant COMInterlocked

Macros: COMPlusThrow seires

AaronRobinsonMSFT commented 1 year ago

Thanks @huoyaoyuan

A list of symbols that can be renamed at my site of view:

From that list, the FCall related changes have a mix of real COM interop and just legacy naming. The COM interop related will need some more scrutiny. The exception macros can be changed for sure.

The COMVariant, COMOAVariant, ComAwareWeakReferenceNative and COMDateTime are a bit tricky for now and a mix of real COM types/specifications and just general .NET code. They should be left alone in this initial effort.

Giviruk commented 6 months ago

can I try to do this issue? do I just need to rename the types from the list or something else?

AaronRobinsonMSFT commented 6 months ago

@Giviruk Sure. Please note the guidance from https://github.com/dotnet/runtime/issues/86145#issuecomment-1571326810.

huoyaoyuan commented 6 months ago

I'd suggest to go with less types at a time, and exclude ones when you feel it's too complex. You can start with searching the usages.

pierrebelin commented 5 months ago

can I try to do this issue? do I just need to rename the types from the list or something else?

I'd like to work on this one, do you need any help ?

Giviruk commented 5 months ago

Unfortunately, I was never able to start working on the task, so someone else can start doing it.

pierrebelin commented 4 months ago

Here is what I understand from the code to resolve this issue:

For COM interop related: ComPlusCall -> CallNative ComPlusCallInfo -> CallInfoNative ComPlusCallMethodDesc -> CallMethodDescNative GenericCLRToCOMCallStub -> GenericCallNativeStub

Should I also rename class like ComPlusMethodFrame to MethodFrameNative ?

For FCallsRelated : COMDelegate -> DelegateNative COMModule -> ModuleNative along with calls like COMDelegate::GetMethodDesc -> ModuleNative_GetMethodDesc as noticed in the PR #60992

comdependenthandle.h -> dependenthandlenative.h comdynamic.h -> dynamicwritenative.h with COMDynamicWrite -> DynamicWriteNative comsynchronizable.h -> synchronizablenative.h comthreadpool.h -> threadpoolnative.h comwaithandle.h -> waithandlenative.h COMDouble -> DoubleNative COMSingle -> SingleNative COMInterlocked -> InterlockedNative COMPlusThrow -> ThrowNative

Am I on the right track? It's only my second issue

huoyaoyuan commented 4 months ago

The Native suffix should only be used for types that serves with corresponding managed type.

ComPlusCall -> CallNative ComPlusCallInfo -> CallInfoNative ComPlusCallMethodDesc -> CallMethodDescNative GenericCLRToCOMCallStub -> GenericCallNativeStub

Should I also rename class like ComPlusMethodFrame to MethodFrameNative ?

There should use ComToClr or ClrToCom.

along with calls like COMDelegate::GetMethodDesc -> ModuleNative_GetMethodDesc as noticed in the PR

Converting should better be a separated PR other than renaming.

COMPlusThrow -> ThrowNative

These are native-only macros. CLRThrow should be better.

pierrebelin commented 4 months ago

Thanks !

I saw inside the solution types already named CRLToCOMWorker, I think it's a good idea to keep it for those usecases: ComPlusCall -> CRLToCOMCall ComPlusCallInfo -> CRLToCOMCallInfo ComPlusCallMethodDesc -> CRLToCOMCallMethodDesc GenericCLRToCOMCallStub -> GenericCRLToCOMCallStub

Perfect, I'll start to rename all on this and create a PR related

huoyaoyuan commented 4 months ago

I still suggest to start with a few cases per PR. Some of these like COMPlusThrow has quite big impact on the whole repo.

pierrebelin commented 4 months ago

No problem, I started with classes with ComPlus of COM interop related

jkotas commented 4 months ago

102922 is not a complete fix for this issue.

syfFerdinand commented 1 week ago

Hello everyone,

I'd like to contribute to this issue and work on renaming the types as suggested. Is there any specific guidance or parts of the list I should start with?

I noticed some previous discussions regarding the complexity of certain types (like COMPlusThrow), so I can begin with the simpler cases to avoid conflicts. Any advice or confirmation on how to proceed would be greatly appreciated.

Thank you!

huoyaoyuan commented 1 week ago

I'm afraid this isn't suitable for beginners. Although completing the renaming isn't complex, it's better to have more knowledge of the internal mechanisms of the coreclr code base to understand the naming conventions, and find all references that can be missed by IDE.

syfFerdinand commented 1 week ago

Thank you for your feedback!

I understand that this issue might require deeper knowledge of the CoreCLR codebase. Given that, I’ll take a look at other issues where I can contribute while learning more about the internals.

If you have any suggestions for issues that might be more suitable for someone at my current level, I’d be happy to work on those as well.

Thanks again for your guidance!