Skip to content

Swift: simplify GetImmediateParent.qll #9398

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

Merged
merged 1 commit into from
Jun 1, 2022
Merged

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Jun 1, 2022

This is a quick follow up to #9380, getting rid of the intermediate class implementation there (by leveraging the fact that we can replace this with e using the fact that it is always the first parameter in the db predicate, and that repeated properties have always the same shape of the parameters to the db predicate).

@redsun82 redsun82 requested a review from MathiasVP June 1, 2022 13:03
@redsun82 redsun82 requested a review from a team as a code owner June 1, 2022 13:03
@github-actions github-actions bot added the Swift label Jun 1, 2022
@redsun82 redsun82 added the no-change-note-required This PR does not need a change note label Jun 1, 2022
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM with one tiny comment.


/**
* Gets any of the "immediate" children of `e`. "Immediate" means not taking into account node resolution: for example
* if the AST child is the first of a series of conversions that would normally be hidden away, this will select the
* next conversion down the hidden AST tree instead of the corresponding fully uncoverted node at the bottom.
* This predicate is mainly intended to be used to test uniqueness of parents.
* Outside this module this file is mainly intended to be used to test uniqueness of parents.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be more aggressive with the wording. For some reason, I can't get Add a suggestion to work 🤔. I'd replace this line with an initial comment saying "INTERNAL: Do not use". You'll see this a lot in QL code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you place that on both the predicates, or only on getAnImmediateChild?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd place it only on getAnImmediateChild as a replacement to the Outside this module this file is mainly intended to be used to test uniqueness of parents. part of the comment.

@MathiasVP MathiasVP merged commit cb7be4f into main Jun 1, 2022
@MathiasVP MathiasVP deleted the redsun82/swift-getparent branch June 1, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants