Skip to content

RQName cleanup#45823

Merged
jasonmalinowski merged 5 commits intodotnet:masterfrom
jasonmalinowski:rqname-cleanup
Jul 9, 2020
Merged

RQName cleanup#45823
jasonmalinowski merged 5 commits intodotnet:masterfrom
jasonmalinowski:rqname-cleanup

Conversation

@jasonmalinowski
Copy link
Member

@jasonmalinowski jasonmalinowski commented Jul 9, 2020

Some cleanup of our RQName generation code.

This partially fixes/addresses #45805 by ensuring we aren't going to be triggering asserts. I'm not going to close that bug quite yet as I'm not sure if we're going to need to update RQNames to support a RQName format for function pointers, but this should make it fairly less broken.

Commit-at-a-time will make it a bit clearer what each step was, but it's straightforward all at once.

This annotation revealed a number of places where if some part of RQName
generation failed, we'd end up creating RQNode objects with null fields;
trying to convert that to the text would end up crashing. This adds
proper error handling when issues were discovered by analysis.
Nothing is using UnresolvedRQNode directly, so we can just flatten the
type hierarchy a bit.
@jasonmalinowski jasonmalinowski requested a review from a team as a code owner July 9, 2020 00:45
@jasonmalinowski jasonmalinowski self-assigned this Jul 9, 2020
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no context, but the annotations make sense to me

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasonmalinowski jasonmalinowski merged commit 65cd374 into dotnet:master Jul 9, 2020
@ghost ghost added this to the Next milestone Jul 9, 2020
@jasonmalinowski jasonmalinowski deleted the rqname-cleanup branch July 9, 2020 18:31
@JoeRobich JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants