Skip to content

[Frontend] Add a new -emit-interface-path option #18090

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 24, 2018
Merged

[Frontend] Add a new -emit-interface-path option #18090

merged 3 commits into from
Jul 24, 2018

Conversation

jrose-apple
Copy link
Contributor

...but don't hook it up to anything yet. Also reorganize the Frontend tests a bit so that I can add new tests in the right place.

This is the very very start of the module stability / textual interfaces feature described at https://forums.swift.org/t/plan-for-module-stability/14551/

For now I've just made it a frontend option (not a driver option), which is good enough for testing.

@jrose-apple
[test] Split out the Frontend parts of test/Driver/options.swift
b88256e 
This dates back to the early days when the Driver was still being
brought up, but there's no reason to put them together now.

Reorganization and elimination of redundancy only.
@jrose-apple
[test] Move tests that only test the Frontend out of Driver/
d176bfa 
Reorganization only.
@jrose-apple jrose-apple requested a review from davidungar July 19, 2018 22:58
@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test

davidungar
davidungar approved these changes Jul 20, 2018
View reviewed changes
Copy link
Contributor

@davidungar davidungar left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I would add another sentence to the comment in SupplementaryOutputPaths.h. And, I didn't inspect the logic in the large switches-by-output-type very closely, but no existing functionality will break.

include/swift/Basic/SupplementaryOutputPaths.h Outdated
@@ -60,6 +60,11 @@ struct SupplementaryOutputPaths {
/// It is currently only used with WMO, but could be generalized.
std::string TBDPath;

/// The path to which we should emit a textual module interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding another sentence here, regarding the purpose of the textual module interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. I'd rather put that on the function that actually generates the thing. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question! Where are the explanations for the other supplementary outputs? How would one find the function that does the generation if one were looking at this declaration? Seems easier to go from the use of the declaration to the declaration than the reverse, because there is a plurality of uses, but only one declaration. I guess I lean towards the declaration, but any choice that makes it discoverable (e.g. cross-references) seems fine.

@jrose-apple jrose-apple force-pushed the emit-interface-path branch from ed3d625 to 982243f Compare July 20, 2018 23:06
@jrose-apple
Copy link
Contributor Author

I still feel a little weird putting the comment here because it doesn't seem like the place to look if you want to know what a textual interface is, but I added a bit of explanation, at least.

@swift-ci Please smoke test

davidungar
davidungar reviewed Jul 20, 2018
View reviewed changes
include/swift/Frontend/Types.def Outdated
@@ -48,6 +48,7 @@ TYPE("dependencies", Dependencies, "d", "")
TYPE("autolink", AutolinkFile, "autolink", "")
TYPE("swiftmodule", SwiftModuleFile, "swiftmodule", "")
TYPE("swiftdoc", SwiftModuleDocFile, "swiftdoc", "")
TYPE("swiftinterface", SwiftModuleInterfaceFile, "swiftinterface", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worthwhile adjusting the spacing on the other lines to maintain the columnar alignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure why I was hesitant to do so. Will fix.

@davidungar
Copy link
Contributor

davidungar commented Jul 20, 2018 •
edited
Loading

I guess I expected the comment in SupplementaryOutputPaths.h to say something about "module stability" or some indication of why this is needed when today modules can be imported without it.

@jrose-apple
Copy link
Contributor Author

Yeah, I don't think that belongs in SupplementaryOutputPaths.h. I don't know where it does belong, but SupplementaryOutputPaths.h defines a little helper structure that holds paths. It's not like Subsystems.h, which points out to a lot of other parts of the compiler. That is, I don't think this is "the declaration" that someone would use to find out what textual interfaces are.

@jrose-apple
Copy link
Contributor Author

I'm about to make emitModuleInterface(raw_ostream &out, ModuleDecl *M) and I think that's where such a comment belongs.

@davidungar
Copy link
Contributor

Fair point, but where else do you go to discover the purpose of each output?

@jrose-apple
[Frontend] Add a new -emit-interface-path option
b9ae66d 
...but don't hook it up to anything yet.

This is the very very start of the module stability / textual
interfaces feature described at

  https://forums.swift.org/t/plan-for-module-stability/14551/

For now I've just made it a frontend option (not a driver option),
which is good enough for testing.
@jrose-apple jrose-apple force-pushed the emit-interface-path branch from 982243f to b9ae66d Compare July 20, 2018 23:41
@jrose-apple
Copy link
Contributor Author

Fair point, but where else do you go to discover the purpose of each output?

I'm not sure. The best answer I have is Options.td, since that's where the options for the various outputs are defined, but we only have the classic terse single lines of help text there.

@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test

@jrose-apple
Copy link
Contributor Author

@davidungar, think this discussion is important enough to hold off on merging? Or should I go ahead?

(Trying not to get too far ahead on my local branch, something I remember from when our roles were reversed!)

@davidungar
Copy link
Contributor

@jrose-apple Thanks for asking. I would be OK with merging as-is, but would really prefer if something like this sentence:
"This is the very very start of the module stability / textual interfaces feature described at https://forums.swift.org/t/plan-for-module-stability/14551/"
were somewhere I could find it with a grep of the new output path name. I realize that SupplementaryOutputPaths.h isn't ideal, but it's better IMO than nowhere. Up to you...

@jrose-apple
Copy link
Contributor Author

All right. I guess the least bad thing is to make SupplementaryOutputPaths the home for that information after all, or at least the links out to other APIs, files in docs/, or forum posts. I'll do that in a follow-up PR.

@jrose-apple jrose-apple merged commit a39afdc into swiftlang:master Jul 24, 2018
@jrose-apple jrose-apple deleted the emit-interface-path branch July 24, 2018 19:49
@jrose-apple jrose-apple mentioned this pull request Jul 24, 2018
Merged
@davidungar
Copy link
Contributor

Thanks!

@jrose-apple jrose-apple mentioned this pull request Jul 25, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidungar davidungar davidungar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jrose-apple @davidungar