Skip to content

Add native integer keyword recommenders#42976

Merged
cston merged 2 commits intodotnet:features/NativeIntfrom
cston:keywords
Apr 2, 2020
Merged

Add native integer keyword recommenders#42976
cston merged 2 commits intodotnet:features/NativeIntfrom
cston:keywords

Conversation

@cston
Copy link
Contributor

@cston cston commented Apr 1, 2020

Relates to #38821 (native ints feature)

@cston cston marked this pull request as ready for review April 1, 2020 04:45
@cston cston requested review from a team as code owners April 1, 2020 04:45
@jcouv jcouv self-assigned this Apr 1, 2020
if (context.IsStatementContext ||
context.IsGlobalStatementContext ||
context.IsPossibleTupleContext ||
context.IsPatternContext ||
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Apr 1, 2020

Choose a reason for hiding this comment

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

  1. can you add a pattern test and a tuple test.
  2. also a test that it doesn't appear here enum e : $$
  3. what aobut here nint n = new $$
  4. not after as
  5. yes after is
  6. after fixed?
  7. in lambda parameter
  8. static operator implicit $$
  9. <see cref="$$"/>
  10. after ref?
  11. after const? (unless it's not legal there). i think you have test for this. but check field and local.
  12. after stackalloc?
  13. return type of a member in a class?
  14. default($$)
  15. not in type parameter constraints.

#Resolved

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Apr 1, 2020

Choose a reason for hiding this comment

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

need a test for a normal expression location. like var v = 1 + $$. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestions, thanks.


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

Copy link
Member

Choose a reason for hiding this comment

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

also a test that it doesn't appear here enum e : $$

I thought LDM agreed this was fine provided it didn't require unreasonable changes? I can't actually find the notes for that meeting however...

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought LDM agreed this was fine provided it didn't require unreasonable changes? I can't actually find the notes for that meeting however...

If so, then add test that it's legal there :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, here we go: https://github.com/dotnet/csharplang/blob/3b22aa0b47cc2ab6352e01f73f976fdbbf9d9b2d/meetings/2019/LDM-2019-10-23.md

For use as an enum underlying type, we think nint/nuint should be allowed as long as it's not too much implementation work. The legal values would be the same as the valid constants for the underlying.

Copy link
Contributor Author

@cston cston Apr 1, 2020

Choose a reason for hiding this comment

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

I'll include the enum context when native integers are supported as underlying types in the compiler. For now, the keyword recommender will not include the keywords in that context.


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

{
protected abstract RecommendedKeyword Keyword { get; }

private bool IsValidContext(CSharpSyntaxContext context)
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Apr 1, 2020

Choose a reason for hiding this comment

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

static? #Resolved

{
internal sealed class NintKeywordRecommender : AbstractNativeIntegerKeywordRecommender
{
protected override RecommendedKeyword Keyword => new RecommendedKeyword("nint");
Copy link
Contributor

Choose a reason for hiding this comment

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

nameof(nint)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can make that change when the feature is included in the build.


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

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Overall looks fine. Just want to flesh out tests more to interesting cases

public void AliasName_02()
{
var source =
@"using N = nint;
Copy link
Member

Choose a reason for hiding this comment

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

Why does this allowed, but using I = int; isn't? Seems inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. #42975


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

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.

Compiler change (test-only) LGTM Thanks

@jcouv jcouv mentioned this pull request Apr 1, 2020
83 tasks
@cston cston merged commit 3dd6470 into dotnet:features/NativeInt Apr 2, 2020
@cston cston deleted the keywords branch April 2, 2020 00:54
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.

4 participants