-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Nullable annotate InvisibleEditor.cs #45978
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
Nullable annotate InvisibleEditor.cs #45978
Conversation
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.
VsTextLines has been revised to never be null.
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's not clear to me why this was only using an 'as' cast to convert the docData to an IVsTextLines. We had a hard cast to IVsTextBuffer a line later, and although it's possible that somebody could implement IVsTextBuffer but not IVsTextLines, I can't think of any reasonable scenario for that. Worse off, the only objects here that can be passed to GetDocumentBuffer and actually work would be the editor implementations anyways which would implement all the interfaces.
I'm figuring this all just evolved this way for no good reason, so I'm cleaning it up.
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.
@davkean The AbtractHostObject stuff would all have been the fake IVsHierarchies you used to create for each target; that's all gone now right? I imagine we can delete this hack entirely.
19f280e to
34fb647
Compare
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.
❔ Where would the previous implementation have thrown if this invariant was violated?
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.
Calling the VS API with an empty file path wouldn't have worked, that much is certain.
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.
❔ How do we know FilePath is not null here? Is there already validation of this condition earlier in the code path?
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.
Ditto: code never could have worked prior. And more practically, remote files are backed by the file system in a certain magic temp directory.
34fb647 to
fb34e59
Compare
ghost
left a comment
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.
Auto-approval
No description provided.