-
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
Use 7.2 language features in Roslyn #26010
Conversation
@dotnet/roslyn-compiler - FYI. Trying to use new language features in Roslyn. |
Ref reassignments work very nicely in the AVL tree implementation of SmallDictionary. 👍 |
Another nice part is that the changes are not disrupting existing code and can be done incrementally. |
|
||
currentNodeParent = currentNode; | ||
currentNode = currentNode.Right; | ||
currentNode = new AvlNode(hashCode, key, value); |
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.
I like this... this is how I usually implement things in C++, it simplifies the code a lot and also removes a special case for inserting the root node.
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.
also removes a special case for inserting the root node
would that be possible here by restructuring the loop and moving the null check?
{ | ||
_triviaStack.PushLeadingTrivia(ref token); | ||
_triviaStack.PushLeadingTrivia(in token); |
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.
❓ Why explicit? At this point it behaves like passing by value which seems to be less noisy. #WontFix
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.
It was explicit ref
originally. I assumed that ref
was used for performance which makes it important that we continue passing the token by direct reference. That is where you use in
at the call site - to make sure there is no copying.
Basically - I used the "ref for performance reasons" pattern as an indication of perf-sensitive codepaths and used explicit in
there.
I think it makes sense to prevent unexpected perf regressions or at least as a self-documenting way to say that avoiding copies is very important. #WontFix
@@ -6,7 +6,7 @@ | |||
|
|||
namespace Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax | |||
{ | |||
internal struct BlendedNode | |||
internal readonly struct BlendedNode |
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.
❓ What prevents this in C# 7.2? If it's allowed, it would be helpful in the review if the C# 7.2 features could be brought in separately from the C# 7.3 features. #Resolved
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.
I did not realize that 7.2 is no longer barred. I will separate 7.3 into a separate PR, so that this could actually get merged.
In reply to: 179906078 [](ancestors = 179906078)
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.
Note that I still need to update the toolset compiler packages to 2.7.0 which matches Visual Studio 2017 version 15.6
There was a number of important fixes and it seems the changed code is relying on some of them.
In reply to: 180535550 [](ancestors = 180535550,179906078)
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.
@dotnet/roslyn-compiler - ok, since we want to merge this, I need one more review. Thanks. |
The 7.3 part is now a separate PR - #26092 That is not going to be merged. Not until after 7.3 is officially released. |
@@ -395,7 +395,7 @@ private TNode[] Nodes | |||
/// </summary> | |||
public Enumerator GetEnumerator() | |||
{ | |||
return new Enumerator(this); | |||
return new Enumerator(in this); |
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.
❓ Why make this change? The current structure appears to only have one field, which is SyntaxNode
(a reference type) and therefore be cheapest to copy by value. #Resolved
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.
Will take a look. I may have mixed it up with other cases. There would not be much point to pass a single reference by in
. #Resolved
@@ -2210,7 +2210,6 @@ Microsoft.CodeAnalysis.SyntaxTokenList.Reverse() -> Microsoft.CodeAnalysis.Synta | |||
Microsoft.CodeAnalysis.SyntaxTokenList.Reversed | |||
Microsoft.CodeAnalysis.SyntaxTokenList.Reversed.Enumerator | |||
Microsoft.CodeAnalysis.SyntaxTokenList.Reversed.Enumerator.Current.get -> Microsoft.CodeAnalysis.SyntaxToken | |||
Microsoft.CodeAnalysis.SyntaxTokenList.Reversed.Enumerator.Enumerator(ref Microsoft.CodeAnalysis.SyntaxTokenList list) -> void |
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.
Breaking change? #ByDesign
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.
Yes, but only with specific compiler versions (or newer): dotnet/csharplang#1308 #ByDesign
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.
(which might be even worse than a normal breaking change 😄) #WontFix
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.
Technically yes. But I am certain that it was public by mistake. Noone should be instantiating these enumerators except via GetEnumerator
call. No other struct enumerator have a public constructor. Especially those that take byre parameters.
We can discuss this further but while most compat breaks are bad, not all are. I think this public exposure should be undone ASAP. - not just because it stands in the way of changing from ref
-> in
, I could leave it as ref
. I left ref
in other public places, for compat reasons.
Here a public ctor just makes no sense.
In reply to: 180876819 [](ancestors = 180876819)
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.
I am simply removing the ctors. It is not related to in
vs. ref
In reply to: 180885818 [](ancestors = 180885818)
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.
I have sent a note to Compat Council to decide on this, but I think we should remove these from the public API.
I can keep the ref
ctors public (for what use?) and add in
ctor with a dummy parameter for the use in GetEnumerator - to avoid defensive copying, but it would be a very counterproductive "fix"
In reply to: 180900004 [](ancestors = 180900004,180876819)
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.
Compat Council is ok with removing the following from the public surface:
-
Microsoft.CodeAnalysis.SyntaxTokenList.Reversed.Enumerator.Enumerator(ref Microsoft.CodeAnalysis.SyntaxTokenList list)
-
Microsoft.CodeAnalysis.SyntaxTriviaList.Reversed.Enumerator.Enumerator(ref Microsoft.CodeAnalysis.SyntaxTriviaList list)
In reply to: 180905813 [](ancestors = 180905813,180900004,180876819)
@dotnet/roslyn-compiler - PING |
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.
LGTM
Why tests are getting stuck and not starting? |
@VSadov I sent an email about this in the AM. 😄 |
@jaredpar I thought the issue was resolved and I may need just cycle through Close/Reopen that typically resolves "stuck" tests issue. |
Oh well.. |
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.
Awesome! 👍
@VSadov looks like the required tests have all passed now. |
Thanks!! |
Trying to use the new language features in the Roslyn codebase.