-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Merge main into features/list-patterns #55072
Conversation
alrz
commented
Jul 23, 2021
•
edited
Loading
edited
Revert "Ensure we don't try and build an empty table when modifying entries"
…elease/dev17.0-to-main
…elease/dev17.0-to-release/dev17.0-vs-deps
…-release/dev17.0-vs-deps Merge release/dev17.0 to release/dev17.0-vs-deps
… into merges/main-to-features/FileScopedNamespaces
@jcouv I've had already started attempting a merge. Feel free to close if you're half way through. |
Cool! There were more merge conflicts than I expected, so I'm glad you took a stab :-) |
Most of it is a removed unused arg which I regret doing it in this branch. |
The only trouble is that we can't really review this PR as-is, as the merge conflicts are lost into the merge commit. I think it's possible to re-do this with miminal effot:
|
Okay. I think that works too. I'll take a look that way |
Actually that doesn't work :-/ GitHub won't let me jump to a specific file, because too many files changed... |
I did a clean merge with "theirs" option to see the diff: https://github.com/dotnet/roslyn/compare/1cfbd6dc..81d9c767 Does that help? |
I'm not sure. I'd still recommend the approach I described earlier. It is tried and tested (one merge commit keeping conflicts, one commit to resolve conflicts). |
81d9c76 is the merge with manually resolved conflicts (I do see this in the PR) The diff basically gives you all manual changes. |
# Conflicts: # src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs # src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs # src/Compilers/CSharp/Portable/Errors/MessageID.cs # src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs # src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs # src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs # src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs # src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_FunctionPointerInvocation.cs # src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs # src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectOrCollectionInitializerExpression.cs # src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt # src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf # src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hant.xlf # src/Compilers/Test/Utilities/CSharp/TestOptions.cs
Done. |
@@ -8051,7 +8051,7 @@ private BoundExpression BindIndexedPropertyAccess(SyntaxNode syntax, BoundExpres | |||
argIsIndex | |||
? ErrorCode.ERR_ImplicitIndexIndexerWithName | |||
: ErrorCode.ERR_ImplicitRangeIndexerWithName, | |||
arguments.Names[0].GetLocation()); | |||
arguments.Names[0].GetValueOrDefault().Location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a null
location what we want for the OrDefault
case, as opposed to Location.None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs
Show resolved
Hide resolved
public static readonly CSharpParseOptions RegularWithExtendedPropertyPatterns = RegularPreview; | ||
>>>>>>> origin/main | ||
public static readonly CSharpParseOptions RegularWithListPatterns = RegularPreview; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this shouldn't be necessary anymore. The default version for all tests has been changed to preview
.
Only LangVer tests will have to do something explicit (using RegularN for old and TemporaryPreview LangVer for new). TemporaryPreview will be replaced by RegularN+1 when the new language version is introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default version for all tests has been changed to preview.
Good to know. I can remove in some other PR (existing or upcoming) if that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like. This is just something to know moving forward. Hopefully it should be a simplification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks much for restructuring the PR. It was much easier to review that way.
Looks good, just a question on possible null Location.