Skip to content

Conversation

mairaw
Copy link
Contributor

@mairaw mairaw commented May 1, 2019

No description provided.

@mairaw mairaw added this to the May 2019 milestone May 1, 2019
@mairaw mairaw self-assigned this May 1, 2019
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Generally LGTM, @mariaw, though I've left two comments about changes that didn't look correct (and I only spot-checked the PR).

@@ -98,7 +98,7 @@
<AssemblyName>System.Core</AssemblyName>
</AssemblyInfo>
<Attributes>
<Attribute FrameworkAlternate="netcore-2.0;netcore-2.1;netcore-2.2;netcore-3.0">
<Attribute FrameworkAlternate="netcore-2.0;netcore-2.1;netcore-2.2">

Choose a reason for hiding this comment

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

This seems suspicious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does indeed. @joelmartinez can you check if this is expected? If not, I can open a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

offhand, no I'm not sure why this occurred ... yes a bug would be ideal

@@ -35,7 +35,7 @@
</ReturnValue>
<Parameters>
<Parameter Name="reader" Type="System.Buffers.SequenceReader&lt;System.Byte&gt;" RefType="this" />
<Parameter Name="value" Type="System.Int64" RefType="out" />
<Parameter Name="value" Type="System.Int16" RefType="out" />

Choose a reason for hiding this comment

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

The changes in this file look problematic. There should be three overloads of TryReadLittleEndian and TryReadBigEndian. They're distinguished by their out parameters: Int16, Int32, and Int64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a "known" issue but I'm not sure if we have a bug tracking these APIs. @joelmartinez can you confirm if you have a tracking bug for this? I couldn't find it on https://github.com/mono/api-doc-tools

Copy link
Contributor

Choose a reason for hiding this comment

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

@mairaw no I'm not sure I had seen this issue before … hmm, so the issue is that there's three overloads in the actual assemblies, but the EcmaXml only shows one (and the type is being updated in this latest update?)

@mairaw mairaw merged commit 89a07a3 into master May 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the CITest branch May 1, 2019 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants