-
Notifications
You must be signed in to change notification settings - Fork 465
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
Fix build warnings and update compiler #3400
Changes from 3 commits
00e13b7
c9e53dd
def6060
93cfc3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,14 +66,14 @@ public sealed override void Initialize(AnalysisContext context) | |
return; | ||
} | ||
|
||
var forwardGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>(); | ||
var invertedGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>(); | ||
ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>> forwardGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please retarget all security rule changes to 2.9.x branch. Also, there is no nullability involved here, so why was this required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not practical to backport this change. The 2.9.x branch doesn't support this toolchain or compiler version so it would be a large backport of much more than just this pull request. I don't believe these changes will be a problem in practice. /cc @dotpaul for awareness. |
||
ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>> invertedGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>(); | ||
|
||
// It keeps the out Degree of every vertex in the invertedGraph, which is corresponding to the in Degree of the vertex in forwardGraph. | ||
var inDegree = new ConcurrentDictionary<ISymbol, int>(); | ||
ConcurrentDictionary<ISymbol, int> inDegree = new ConcurrentDictionary<ISymbol, int>(); | ||
|
||
// It Keeps the out degree of every vertex in the forwardGraph, which is corresponding to the in Degree of the vertex in invertedGraph. | ||
var outDegree = new ConcurrentDictionary<ISymbol, int>(); | ||
ConcurrentDictionary<ISymbol, int> outDegree = new ConcurrentDictionary<ISymbol, int>(); | ||
|
||
compilationStartAnalysisContext.RegisterSymbolAction( | ||
(SymbolAnalysisContext symbolAnalysisContext) => | ||
|
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.
Those explicit types were not necessary, were they?
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 commit introducing these changes is named "Use explicit type where compiler fails to evaluate nullability" so I think they matters.
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.
@jcouv yes, they are necessary.
var
doesn't work with local functions, at least not here.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.
Do we need to file a compiler bug?
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 think there are already bugs filed for improving local function support. It hit Roslyn too.
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#41689 and dotnet/roslyn#41705