Skip to content

Module aliasing: allow getting real module names via ImportPath getters #40064

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 15 commits into from
Nov 11, 2021

Conversation

elsh
Copy link
Contributor

@elsh elsh commented Nov 5, 2021

-Set real module names in ImportDecl
-Add getRealImportPath / getRealModulePath
-Print real module names in ASTDumper
Resolves rdar://83632921, rdar://83633109

@elsh elsh requested review from xymus and beccadax November 5, 2021 09:02
@elsh elsh changed the title Module aliasing: modify ImportPath getters to return real module names Module aliasing: allow getting real module names via ImportPath getters Nov 5, 2021
@elsh
Copy link
Contributor Author

elsh commented Nov 6, 2021

@swift-ci smoke test

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

I noted a few nitpicks but overall the logic looks good to me.

Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

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

You might be able to talk me out of it, but I strongly suspect this should be implemented a little differently. Functionally, though, I think this is great other than the test quirks @xymus identified.

@elsh
Copy link
Contributor Author

elsh commented Nov 9, 2021

You might be able to talk me out of it, but I strongly suspect this should be implemented a little differently. Functionally, though, I think this is great other than the test quirks @xymus identified.

Thanks for the suggestions @beccadax! Addressed the rest of the comments.

@elsh
Copy link
Contributor Author

elsh commented Nov 9, 2021

@beccadax @xymus Thanks for reviewing! Let me know if there's anything else that needs to be addressed.

@elsh elsh force-pushed the es-imports branch 3 times, most recently from 374f470 to 41c3d78 Compare November 10, 2021 00:51
@elsh
Copy link
Contributor Author

elsh commented Nov 10, 2021

@swift-ci smoke test

@elsh
Copy link
Contributor Author

elsh commented Nov 10, 2021

@swift-ci smoke test

1 similar comment
@elsh
Copy link
Contributor Author

elsh commented Nov 10, 2021

@swift-ci smoke test

Copy link
Contributor

@beccadax beccadax 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 very, very minor comments on formatting and comment wording. (Some are mistakes in my previous suggestions—sorry about that.) These are all simple and mechanical, so I'm going to mark this "Approve" so that you don't need to ask for another review.

@elsh
Copy link
Contributor Author

elsh commented Nov 10, 2021

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit 71cdfff into main Nov 11, 2021
@swift-ci swift-ci deleted the es-imports branch November 11, 2021 02:57
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.

4 participants