-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Automatically link against Core #6362
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
base: trunk
Are you sure you want to change the base?
Conversation
|
AFAIK @chandlerc was working on refactoring the code for runtimes, so he might need to review this. |
|
Also, I might be missing something with adding the defaults to the structs as some other options have defaults, but all the functionality appears to be the same. |
chandlerc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being a bit slow to get back here, left some initial comments.
| "carbon_runner.cpp", | ||
| "carbon_runner.h", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be split into a separate carbon_runner library? Is there something that goes wrong with that?
| friend CarbonRunner; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? It doesn't seem like it should be...
| private: | ||
| auto Compile(llvm::SmallVector<std::filesystem::path> input_filenames, | ||
| Lower::OptimizationLevel opt_level, llvm::Triple target, | ||
| bool prelude_import) -> DriverResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't expect this to be private, this seems like it should be part of the public API?
|
|
||
| // Helper class for cross-subcommand functionality. | ||
| // Allows type-safe calls of subcommands. | ||
| // TODO: May need to be adjusted once caching is refactored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the refactorings are now landed, so hopefully can update all of this and address the TODOs.
This adds a
CarbonRunnerclass that (for now) builds an archive of the compiled Core libs in thecarbon_runtimescache.It also modifies
carbon build-runtimesto call this functionality andcarbon linkto automatically use the archive during linking.Cross-subcommand functionality is confined to the
CarbonRunnerclass as future work will probably require a lot more crossing (i.e.carbon buildwill probably callcompileandlink, andcarbon compilewill probably need precompiled metadata to be built bycarbon build-runtimes).To do this in a type-safe manner, constructors were added for subcommand objects that use the associated options struct and some (missing?) defaults were added to the structs.
These defaults don't change behavior as they were already defaulted in the command line parser, but not in the struct itself.
Most of the constructors are not yet used (only compile is used now) but they may be useful for later.