Error on multiple calling conventions in modopts #39809
Conversation
|
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. |
| @@ -524,25 +526,35 @@ private bool TryGetUnmanagedCallingConventionFromModOpt(MethodSignature signatur | |||
| if (defType.Namespace != "System.Runtime.CompilerServices") | |||
| continue; | |||
|
|
|||
| // Take the first recognized calling convention in metadata. | |||
| void SetCallConv(CorInfoCallConv toSet) | |||
There was a problem hiding this comment.
This is very clever. I am personally not a fan, but defer to the crossgen2 owners.
There was a problem hiding this comment.
The convention for functions like this is to put them after the return statement of the function wherever possible. When you embed them in the middle of the function its easy to think they are part of the natural program flow.
Also, its very unclear for a reader where callConvLocal gets set. I would make that a ref or out parameter to the helper function, if only to make it clear about how the data flows through the function.
| @@ -524,25 +526,35 @@ private bool TryGetUnmanagedCallingConventionFromModOpt(MethodSignature signatur | |||
| if (defType.Namespace != "System.Runtime.CompilerServices") | |||
| continue; | |||
|
|
|||
| // Take the first recognized calling convention in metadata. | |||
| void SetCallConv(CorInfoCallConv toSet) | |||
There was a problem hiding this comment.
The convention for functions like this is to put them after the return statement of the function wherever possible. When you embed them in the middle of the function its easy to think they are part of the natural program flow.
Also, its very unclear for a reader where callConvLocal gets set. I would make that a ref or out parameter to the helper function, if only to make it clear about how the data flows through the function.
| switch (defType.Name) | ||
| { | ||
| case "CallConvCdecl": | ||
| callConv = CorInfoCallConv.CORINFO_CALLCONV_C; | ||
| return true; | ||
| SetCallConvLocal(CorInfoCallConv.CORINFO_CALLCONV_C, ref callConvLocal); |
There was a problem hiding this comment.
Nit: Not a big fan of the local function. It creates a capture object with an unnecessary allocation and I'm not sure if this improves readability over something like:
CorInfoCallConv? callconvLocal = defType.Name switch
{
"CallConvCdecl" => CorInfoCallConv.CORINFO_CALLCONV_C,
// ...
_ => null
};
if (callConvLocal.HasValue)
{
if (found)
throw...
callConv = callConvLocal.Value;
found = true;
}There was a problem hiding this comment.
I'm honestly not a huge fan either. Happy to change it to what people think would be better.
|
The two 'in progress' checks show as completed and successful on Azure DevOps. @davidwrighton I removed the local function and used @MichalStrehovsky's suggestion. Do you have any additional concerns with this change? |
ghost
locked as resolved and limited conversation to collaborators
For Roslyn, the union of the calling conventions specified in
modopts is used and the order doesn't matter. This means that the runtime should not try to choose one to respect based on the order in metadata. This change makes the runtime throwNotSupportedExceptionwhen encountering anunmanagedcalling convention and finding multiple calling conventions specified inmodopts on the return type.Resolves #39378
cc @AaronRobinsonMSFT @jkoritzinsky @davidwrighton