-
Notifications
You must be signed in to change notification settings - Fork 226
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
Razor mappings: Ensure we check if the mapped span is valid #9305
Conversation
41ad553
to
3206e5a
Compare
|
|
&& (!GeneratedCodeRecognizer.IsRazor(token.SyntaxTree) || mappedLineSpan.HasMappedPath) | ||
&& IsInSameFile(mappedLineSpan) |
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 assume that in razor files, if mappedLineSpan.HasMappedPath
is false
, IsInSameFile(mappedLineSpan)
is false
as well. Therefore the mappedLineSpan.HasMappedPath
check might be redundant. Can you confirm this? If so, I would remove
&& (!GeneratedCodeRecognizer.IsRazor(token.SyntaxTree) || mappedLineSpan.HasMappedPath) | |
&& IsInSameFile(mappedLineSpan) | |
&& IsInSameFile(mappedLineSpan) |
and add a comment about the mappedLineSpan.HasMappedPath
being redundant.
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.
You are right, but I would keep it since it is way faster to check this than to do string comparisons (although we cannot completely remove those).
So we reduce the data set a bit here, and later even 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.
LGTM
Related to #9288