Skip to content

Remove IgnoreInsignificantNullableModifiersDifference#34096

Merged
jcouv merged 2 commits intodotnet:masterfrom
jcouv:remove-flag
Mar 14, 2019
Merged

Remove IgnoreInsignificantNullableModifiersDifference#34096
jcouv merged 2 commits intodotnet:masterfrom
jcouv:remove-flag

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Mar 13, 2019

Now that NullableAnnotation has three states, there is no longer a concept of "insignificant nullable difference". Removing that TypeCompareKind.

@jcouv jcouv added this to the 16.1.P1 milestone Mar 13, 2019
@jcouv jcouv self-assigned this Mar 13, 2019
@jcouv jcouv marked this pull request as ready for review March 14, 2019 00:06
@jcouv jcouv requested a review from a team as a code owner March 14, 2019 00:06
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 14, 2019

@dotnet/roslyn-compiler for second review. Thanks

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 14, 2019

Choose a reason for hiding this comment

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

else if ((comparison & TypeCompareKind.IgnoreInsignificantNullableModifiersDifference) == 0) [](start = 20, length = 92)

Shouldn't we keep this block instead and adjust it assuming that the condition is always true (the bit is never set)? #Closed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed. Thanks
I think that both branches behaved the same though, since there's no insignificant differences anymore. So I don't think we can observe the difference.

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Mar 14, 2019

Done with review pass (iteration 1) #Closed

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 2)

@jcouv jcouv merged commit fb27fd7 into dotnet:master Mar 14, 2019
@jcouv jcouv deleted the remove-flag branch March 14, 2019 23:45
333fred added a commit to 333fred/roslyn that referenced this pull request Mar 19, 2019
* dotnet/master: (345 commits)
  Update indexers based on analyzer receiver (dotnet#34134)
  Skip test DecimalBinaryOp_03 (dotnet#34199)
  Remove earlier nullable documentation (dotnet#34153)
  Rewrite FindReferencesTests as theories
  Apply a hang mitigating timeout to ExecuteCommand
  Warn on __refvalue null dereference: (dotnet#34135)
  Update dependencies from https://github.com/dotnet/arcade build 20190312.7 (dotnet#34112)
  Update vs branch for 16.1
  Fix struct layout error when nullable enabled: (dotnet#34128)
  Remove IgnoreInsignificantNullableModifiersDifference (dotnet#34096)
  Add the correct nullable annotations to generated iterator code (dotnet#33986)
  Move Rename implementation to new fully loaded document API.
  handle encapsulate field command
  change the way extract method handle partial load
  handle orangize document
  Add back Go to definition method.
  Revert "Move Go to definition to new fully loaded document API."
  Revert "Move Find references implementation to new fully loaded document API."
  Move Find references implementation to new fully loaded document API.
  Track nullable state across boxing conversions (dotnet#34087)
  ...
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.

3 participants