Skip to content

Commit

Permalink
Pass vectors of raw pointers to OptionProcessor helpers instead of ve…
Browse files Browse the repository at this point in the history
…ctors of unique_ptrs.

Passing a vector<unique_ptr<T>>& is bad for two reasons:
1) Conceptual - unique_ptr is meant to be a clear signal of ownership transfer. But this is a *reference* to a vector of unique_ptrs so ownership isn't transferred.
2) Practical - this interface is impossible to work with unless you happen to already have a vector of unique_ptrs, because they're move-only so you can't create this on the fly.

RELNOTES: None.
PiperOrigin-RevId: 329760925
  • Loading branch information
Googler authored and copybara-github committed Sep 2, 2020
1 parent 80bfa26 commit 9483392
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 11 deletions.
20 changes: 12 additions & 8 deletions src/main/cpp/option_processor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -488,15 +488,20 @@ blaze_exit_code::ExitCode OptionProcessor::ParseOptions(
}
}

// The helpers expect regular pointers, not unique_ptrs.
std::vector<RcFile*> rc_file_ptrs;
rc_file_ptrs.reserve(rc_files.size());
for (auto& rc_file : rc_files) { rc_file_ptrs.push_back(rc_file.get()); }

// Parse the startup options in the correct priority order.
const blaze_exit_code::ExitCode parse_startup_options_exit_code =
ParseStartupOptions(rc_files, error);
ParseStartupOptions(rc_file_ptrs, error);
if (parse_startup_options_exit_code != blaze_exit_code::SUCCESS) {
return parse_startup_options_exit_code;
}

blazerc_and_env_command_args_ =
GetBlazercAndEnvCommandArgs(cwd, rc_files, GetProcessedEnv());
GetBlazercAndEnvCommandArgs(cwd, rc_file_ptrs, GetProcessedEnv());
return blaze_exit_code::SUCCESS;
}

Expand Down Expand Up @@ -538,15 +543,14 @@ void OptionProcessor::PrintStartupOptionsProvenanceMessage() const {
}

blaze_exit_code::ExitCode OptionProcessor::ParseStartupOptions(
const std::vector<std::unique_ptr<RcFile>> &rc_files,
std::string *error) {
const std::vector<RcFile*> &rc_files, std::string *error) {
// Rc files can import other files at any point, and these imported rcs are
// expanded in place. Here, we isolate just the startup options but keep the
// file they came from attached for the option_sources tracking and for
// sending to the server.
std::vector<RcStartupFlag> rcstartup_flags;

for (const auto& blazerc : rc_files) {
for (const auto* blazerc : rc_files) {
const auto iter = blazerc->options().find("startup");
if (iter == blazerc->options().end()) continue;

Expand Down Expand Up @@ -637,7 +641,7 @@ static std::vector<std::string> GetProcessedEnv() {
// BlazeOptionHandler.INTERNAL_COMMAND_OPTIONS!
std::vector<std::string> OptionProcessor::GetBlazercAndEnvCommandArgs(
const std::string& cwd,
const std::vector<std::unique_ptr<RcFile>>& blazercs,
const std::vector<RcFile*>& blazercs,
const std::vector<std::string>& env) {
// Provide terminal options as coming from the least important rc file.
std::vector<std::string> result = {
Expand All @@ -656,7 +660,7 @@ std::vector<std::string> OptionProcessor::GetBlazercAndEnvCommandArgs(
// is reserved the "client" options created by this function.
int cur_index = 1;
std::map<std::string, int> rcfile_indexes;
for (const auto& blazerc : blazercs) {
for (const auto* blazerc : blazercs) {
for (const std::string& source_path : blazerc->canonical_source_paths()) {
// Deduplicate the rc_source list because the same file might be included
// from multiple places.
Expand All @@ -669,7 +673,7 @@ std::vector<std::string> OptionProcessor::GetBlazercAndEnvCommandArgs(
}

// Add RcOptions as default_overrides.
for (const auto& blazerc : blazercs) {
for (const auto* blazerc : blazercs) {
for (const auto& command_options : blazerc->options()) {
const string& command = command_options.first;
// Skip startup flags, which are already parsed by the client.
Expand Down
5 changes: 2 additions & 3 deletions src/main/cpp/option_processor.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ class OptionProcessor {
// server to configure blazerc options and client environment.
static std::vector<std::string> GetBlazercAndEnvCommandArgs(
const std::string& cwd,
const std::vector<std::unique_ptr<RcFile>>& blazercs,
const std::vector<RcFile*>& blazercs,
const std::vector<std::string>& env);

// Finds and parses the appropriate RcFiles:
Expand All @@ -138,8 +138,7 @@ class OptionProcessor {

private:
blaze_exit_code::ExitCode ParseStartupOptions(
const std::vector<std::unique_ptr<RcFile>>& rc_files,
std::string* error);
const std::vector<RcFile*>& rc_files, std::string* error);

// An ordered list of command args that contain information about the
// execution environment and the flags passed via the bazelrc files.
Expand Down

0 comments on commit 9483392

Please sign in to comment.