Skip to content

Simplify NativeIntegerAttribute encoding#46522

Merged
RikkiGibson merged 4 commits intodotnet:masterfrom
cston:44086
Aug 6, 2020
Merged

Simplify NativeIntegerAttribute encoding#46522
RikkiGibson merged 4 commits intodotnet:masterfrom
cston:44086

Conversation

@cston
Copy link
Contributor

@cston cston commented Aug 3, 2020

Encode a bool value for each System.IntPtr and System.UIntPtr only, and ignore custom modifiers.

Fixes #44086.

@cston cston requested a review from a team as a code owner August 3, 2020 20:09
@cston cston added this to the 16.8 milestone Aug 3, 2020
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

@jcouv
Copy link
Member

jcouv commented Aug 3, 2020

Is there a corresponding update to design/spec doc? #Resolved

@jcouv jcouv self-assigned this Aug 3, 2020
@jaredpar
Copy link
Member

jaredpar commented Aug 4, 2020

Do we have a write up for what the encoding is here so that others can use it? Similar to what we have for NRT

https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-metadata.md #Resolved

@cston
Copy link
Contributor Author

cston commented Aug 4, 2020

Do we have a write up for what the encoding is here

Added docs/features/NativeIntegerAttribute.md to the PR.

@333fred
Copy link
Member

333fred commented Aug 5, 2020

Done review pass (commit 2). I didn't fully review the tests, as I'm unsure what the IL actually represents. #Closed

comp.VerifyDiagnostics();

var type = comp.GetTypeByMetadataName("B");
Assert.Equal("void B.F0(nint x, nuint y)", type.GetMember("F0").ToDisplayString(FormatWithSpecialTypes));
Copy link
Member

@333fred 333fred Aug 6, 2020

Choose a reason for hiding this comment

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

Consider verifying these on both preview and 8 compilations. #Resolved

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 3). One minor comment about additional testing.

@RikkiGibson
Copy link
Member

@cston is this ready to merge?

@RikkiGibson RikkiGibson merged commit 4aaaed7 into dotnet:master Aug 6, 2020
@ghost ghost modified the milestones: 16.8, Next Aug 6, 2020
@cston cston deleted the 44086 branch August 6, 2020 22:26
333fred added a commit that referenced this pull request Aug 10, 2020
…features/function-pointers

* upstream/master: (465 commits)
  Stop specifying Rich Nav's version of dotnet
  Fix nullable reference warning
  Fix up some calls to SymbolKey.Resolve now that it's annotated
  Remove try/catch of ArgumentException
  Null annotate SymbolKey
  Annotate Location.IsInSource as ensuring SourceTree is non-null
  Fix nullable annotation of Compilation.CreateErrorTypeSymbol
  Update null state for add/remove on field-like events (#46477)
  Throw IndexOutOfRangeException from BitVector.this[int] if index < 0 (#46627)
  Delete some dead support for changing legacy options
  Delete our use of Microsoft.VisualStudio.CodingConventions
  Fix crash in the preview of a code action that modified an .editorconfig
  Move null check above where the variable is used (#46558)
  Add integration test to configure diagnostic severity via editorconfig
  Disable a few Mac tests
  Consider nullability in conversion to constraint type (#46405)
  Added interpolated strings and tests
  Introduce request context (#46557)
  Simplify NativeIntegerAttribute encoding (#46522)
  Update extension getenumerator status. (#46607)
  ...
@RikkiGibson RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020
@cston
Copy link
Contributor Author

cston commented Sep 6, 2020

Is there a corresponding update to design/spec doc?

dotnet/csharplang#3856


In reply to: 668256851 [](ancestors = 668256851)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider streamlining flag encoding for native integers

5 participants