Skip to content

Update constructor TypeToTypeInfoMarshaler() signature to static #2536

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jun 5, 2019
Merged

Update constructor TypeToTypeInfoMarshaler() signature to static #2536

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a31dd2a
Update constructor TypeToTypeInfoMarshaler() signature to static
EugeneKramer Jun 3, 2019
f9228f3
Public static constructor -> static constructor
EugeneKramer Jun 3, 2019
4e262dd
Update xml/System.Runtime.InteropServices.CustomMarshalers/TypeToType…
EugeneKramer Jun 4, 2019
0d05d50
Update xml/System.Runtime.InteropServices.CustomMarshalers/TypeToType…
EugeneKramer Jun 4, 2019
21f240a
Update constructor EnumerableToDispatchMarshaler() signature to publi…
EugeneKramer Jun 5, 2019
1032c2d
Update constructor EnumeratorToEnumVariantMarshaler() signature to pu…
EugeneKramer Jun 5, 2019
c070d56
Update constructor ExpandoToDispatchExMarshaler() signature to static…
EugeneKramer Jun 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions xml/System.Runtime.InteropServices.CustomMarshalers/TypeToTypeInfoMarshaler.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@
</Docs>
<Members>
<Member MemberName=".cctor">
<MemberSignature Language="C#" Value="public TypeToTypeInfoMarshaler ();" />
<MemberSignature Language="C#" Value="public static TypeToTypeInfoMarshaler ();" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really a public static constructor? I don't have .NET 1.1 (API browser claims that it's a latest version where this member is available), but in modern environment one cannot declare static constructor as public in C#, it generates compiler error CS0515.

Is there even a reason why API Docs should include static constructor? It is not really an "API", in a sence that one cannot use it directly. Moreover, currently the docs do not seem to provide any useful information about it, besides the fact it was removed in .NET 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct. It's a static constructor for a public class. I've removed the public access modifier for the C# and VB.Net static constructor. C++ does not support static constructors, so I've reverted the addition of the static modifier.

While a static constructor cannot be accessed directly, it is called prior to any first instance/usage of the type, which can lead to unusual exceptions such as TypeInitializationException which explicitly note "What often makes this exception difficult to troubleshoot is that static constructors are not always explicitly defined in source code." As such, the presence of a static constructor should be noted in the documentation.

A deprecated static constructor is listed for each of the 4 classes in the System.Runtime.InteropServices.CustomMarshalers Namespace. A separate page for the static constructors is probably unnecessary, but their presence should be noted as a remark.

Appreciate the feedback!

Copy link

Choose a reason for hiding this comment

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

Thanks for submitting this PR and raising the issue of a static constructor appearing in our doc set, @EugeneKramer. Since method signatures are gathered using reflection, I'm not sure that the changed signature won't be overwritten by the next reflection. But I'm also not sure why a static constructor is documented in the first place -- though I don't recall whether 2.0 introduced a change in the way static constructors were handled from 1.0/1.1.

@dend @joelmartinez, do you have any sense of why this constructor is marked as public and appears in the documentation? My guess would be that this is the only static constructor documented in the managed reference.,

I would think that the ILAsm signature also needs to change, although I no longer have any .NET Framework 1.1 assemblies.

For the moment, I'll mark this PR as blocked.

Copy link
Contributor

@mairaw mairaw Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

MSDN also had this constructor:
https://msdn.microsoft.com/en-us/library/03faz537(v=vs.80).aspx?toc=xxx

with the same signature that @EugeneKramer is proposing.

@rpetrusha you can access any version of the assemblies at https://apidrop.visualstudio.com/_git/binaries?path=%2Fdotnet&version=GBmaster

Copy link
Contributor

Choose a reason for hiding this comment

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

apisof.net also lists this constructor, so I think it's fine to have it if we fix the signature
https://apisof.net/catalog/System.Runtime.InteropServices.CustomMarshalers.TypeToTypeInfoMarshaler..cctor()

Copy link

Choose a reason for hiding this comment

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

I still find this bizarre, but the static constructor is clearly public in .NET Framework 1.1. So correcting the syntax is appropriate. Is the correct syntax likely to be overwritten, @mairaw?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the correct syntax likely to be overwritten

Yes ... this needs to be fixed in mdoc's signature formatters. It will reset these next time an update is run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpetrusha - As mentioned earlier, there are public static constructors listed for each of the 4 classes in the System.Runtime.InteropServices.CustomMarshalers Namespace with the same public signature. If this PR is going through, should each of their signatures be updated...or should a separate PR be opened?

System.Runtime.InteropServices.CustomMarshalers Namespace:

Provides the static class constructor.

  1. EnumerableToDispatchMarshaler Constructor
  2. EnumeratorToEnumVariantMarshaler Constructor
  3. ExpandoToDispatchExMarshaler Constructor
  4. TypeToTypeInfoMarshaler Constructor

All were deprecated in .Net 2.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

@joelmartinez I've opened an internal mdoc bug for this:
Bug 97094: [mdoc] wrong signature for public static constructors in .NET Framework 1.1

<MemberSignature Language="ILAsm" Value=".method public static specialname rtspecialname void .cctor() cil managed" />
<MemberSignature Language="DocId" Value="M:System.Runtime.InteropServices.CustomMarshalers.TypeToTypeInfoMarshaler.#cctor" />
<MemberSignature Language="VB.NET" Value="Public Sub New ()" />
<MemberSignature Language="VB.NET" Value="Public Shared Sub New ()" />
<MemberSignature Language="C++ CLI" Value="public:&#xA; TypeToTypeInfoMarshaler();" />
<MemberType>Constructor</MemberType>
<AssemblyInfo>
Expand Down Expand Up @@ -280,4 +280,4 @@
</Docs>
</Member>
</Members>
</Type>
</Type>