Skip to content
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

C#: Move qualified name computation into QualifiedName.qll #14657

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 1, 2023

This PR moves the logic for computing fully qualified names into QualifiedName.qll, and makes it parameterized on how to print unbound generics. This is in preparation for a change to the Models-as-Data format for C#.

@github-actions github-actions bot added the C# label Nov 1, 2023
@hvitved hvitved force-pushed the csharp/qualified-name branch from c5c4295 to 399327e Compare November 1, 2023 14:22
@hvitved hvitved force-pushed the csharp/qualified-name branch from 399327e to c717e34 Compare November 1, 2023 15:22
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Nov 2, 2023
@hvitved hvitved marked this pull request as ready for review November 2, 2023 08:04
@hvitved hvitved requested a review from a team as a code owner November 2, 2023 08:04
tamasvajk
tamasvajk previously approved these changes Nov 2, 2023
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

LGTM, added some questions.

Comment on lines 70 to 72
vort instanceof VoidType and
qualifier = "System" and
name = "Void"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to special case VoidType?
Couldn't the below cover it?

          not exists(vort.getDeclaringType()) and
          qualifier = vort.getNamespace().getFullName() and
          name = vort.getUndecoratedName()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps; I just copied over the existing logic.

Comment on lines 84 to 85
qualifier = "System" and
name = "Nullable<" + getFullName(vort.(NullableType).getUnderlyingType()) + ">"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? NullableType is a subclass of ConstructedType, couldn't the below any(ConstructedType ct | ... cover nullable types too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I just copied it from the existing setup, but I can try to remove it.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Looks good to me! Also great to collect all the logic printing the qualified name in one place.

@hvitved hvitved merged commit f82f1df into github:main Nov 2, 2023
15 checks passed
@hvitved hvitved deleted the csharp/qualified-name branch November 2, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants