Skip to content

Swift: Add and use ApplyExpr.getArgumentByParamName. #11036

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 4 commits into from
Nov 5, 2022

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Oct 28, 2022

Add and use a predicate ApplyExpr.getArgumentByParamName (please say if you can think of a cleaner name for it). This makes it easier to get an argument by its parameter name, rather than the argument name (which is often anonymous) or index (usually the preferred method, but sometimes varies across the functions of interest).

This will be used in a couple of upcoming queries in addition to the two I've updated here.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Oct 28, 2022
@geoffw0 geoffw0 requested a review from a team as a code owner October 28, 2022 16:58
@d10c
Copy link
Contributor

d10c commented Oct 29, 2022

What happens when a parameter has a default value, and an ApplyExpr omits an argument for that parameter? Do you think the parameter's default expression should be the argument in that case?

@geoffw0
Copy link
Contributor Author

geoffw0 commented Oct 31, 2022

Good point, yes, that seems like a reasonable thing to do. In which case I think I'm going to need to add a test for this...

@d10c
Copy link
Contributor

d10c commented Nov 1, 2022

As for naming, getArgumentByParamName sounds fine. Alternatives: getArgumentNamed, getArgumentByName.

@MathiasVP
Copy link
Contributor

Hmm. Slightly related comment: why are we using the name of the parameter and not just its index? The internal parameter name seems like something that the developers are free to change, and it's a bit scary that we're depending on this name

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 1, 2022

why are we using the name of the parameter and not just its index?

There have been a couple of cases where there's a reasonably large collection of functions with the same name and similar parameters, but the target parameter is not at a consistent position. So expressing it by parameter name is more concise and probably more robust. I think using the index is preferred in most other cases.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 1, 2022

... see also upcoming PRs #10947 and #10993 . Now that you mention it the two already in the main branch are not great examples as they would work just as well with indices.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 1, 2022

I've changed the two existing cases to use argument indexes instead. I'm now unsure whether we want ApplyExpr.getArgumentByParamName (probably renamed and with a default values fix) for the upcoming PRs or not.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 3, 2022

OK, in the interests of having fewer balls in the air I've removed the addition of ApplyExpr.getArgumentByParamName from this PR, all it does now is convert two places where parameter names were being matches to used to use argument indexes instead. Lets get this merged + closed.

We can have another go at a getArgumentByParamName at a later point if we decide we really do want it - but with a better name + tests including cases with default arguments.

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! Since this is changing code that needed some join-order assistance in the past I think we should run DCA on it before merging it.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 4, 2022

DCA run is finished. Some stage timing results (quite a lot actually - and apparently not in a good way - but I don't know how to read these yet) . Overall analysis time is unchanged.

@MathiasVP
Copy link
Contributor

(I'm currently looking at a bugfix for the stage timing. Don't worry about those for now)

@MathiasVP MathiasVP merged commit 60ac031 into github:main Nov 5, 2022
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.

3 participants