Skip to content
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

Break out driver environment info into its own type. #4299

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Sep 11, 2024

I want to split commands out so that we don't keep piling onto driver (particularly as I'm eyeing clang-related commands). This extracts out the DriverEnv so that it can be easily shared, with the CompilationUnit as an example.

CARBON_VLOG_TO is to remove the vlog_stream_ requirement of CARBON_VLOG.

@@ -789,7 +792,7 @@ class Driver::CompilationUnit {
!input_filename_.starts_with(options_.exclude_dump_file_prefix);
}

Driver* driver_;
DriverEnv& driver_env_;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer fields not to have a reference type, particularly non-const. Can this be made into a pointer instead? I see that some other fields are references too -- what is the motivation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally I think there's a mix in the toolchain, tending more to references than pointers. One constraint of references is that they're implicitly non-null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps as a stronger argument, three lines down there's a &. This type should be self-consistent.

https://discord.com/channels/655572317891461132/821113559755784242/1283516297686286377 regarding this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to read a possible conclusion from current discussion, updated to DriverEnv*

@josh11b josh11b enabled auto-merge September 11, 2024 23:51
@josh11b josh11b added this pull request to the merge queue Sep 12, 2024
Merged via the queue into carbon-language:trunk with commit de57c99 Sep 12, 2024
8 checks passed
@jonmeow jonmeow deleted the driverenv branch September 12, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants