Restructure NameResolution and fix bug around global#4471
Restructure NameResolution and fix bug around global#4471KevinRansom merged 1 commit intodotnet:masterfrom
Conversation
forki
force-pushed
the
restructure
branch
10 times, most recently
from
c23ec2b to
1104d14
Compare
There was a problem hiding this comment.
This looks good, no need to map when longId is empty
There was a problem hiding this comment.
These look good short-circuiting on empty, I wonder if a empty check on modrefs would improve the other side.
There was a problem hiding this comment.
This looks good moving eAccessRights where needed
There was a problem hiding this comment.
What does the introduced bool represent?
There was a problem hiding this comment.
it's the new first parameter - needed to exclude the global after dot bug
There was a problem hiding this comment.
So this eventuality never arrises now as an id is always passed
There was a problem hiding this comment.
I wonder if splitting the id/rest will allow new code paths to be ran that were not before? Thinking about it logically the id should always be present and what your are doing is removing unrepresentable state.
There was a problem hiding this comment.
yes we move that decision to outer scope. and don't check over and over again in rec functions
|
@forki Does profiling show that the pattern matching in the name resolution code is a hot spot? Just wondering what's behind this :) |
|
@dsyme the additional pattern matching is mostly just null cheks. I don't think that perf is gained here (maybe a very very small bit by reducing string comparison). But I'm trying to reduce InternalError cases and fix bugs as shown in the description above. |
forki
force-pushed
the
restructure
branch
3 times, most recently
from
e831a89 to
3f823eb
Compare
forki
changed the title
|
ready for review |
dsyme
left a comment
dsyme
left a comment
There was a problem hiding this comment.
There is lots of nice cleanup here, thanks
4 participants
globalmodifier like:Also I'm obsessed with NameRes