Refactor Rcfile to use modern Google C++ practices
* Prefer `absl::flat_hash_map` to `std::unordered_map`. This is a much more efficient data structure.
* Prefer `absl::string_view` to `const char*`. This allows compile-time strlen.
* Prefer `absl::WrapUnique` to `std::unique_ptr<T>(T*)`. This better signals intent to wrap a raw pointer.
* Prefer `std::vector` to `std::deque` given we only need a stack, not a queue. This is much faster and more memory-efficient.
* Use if-init to more tightly scope variables.
* Prefer `absl::StrCat` to `blaze_util::StringPrintf` in simple cases. This is faster and more readable when merely concatenating a few strings together.
* Prefer `absl::StrFormat` to `blaze_util::StringPrintf` in cases where we need to do more complicated string construction. This is the standard printf-style library in Google.
* Prefer `absl::StrAppend` to `std::string::operator+`. This is more efficient and readable.
* Prefer `absl::StrSplit` to `blaze_util::Split`. This is the standard string splitting library in Google.
* Prefer `absl::StartsWith` to `std::string::compare`. This is a more expressive API.
* Prefer `absl::c_linear_search` to `std::find`. This avoids needing to name the container three times. This should really be `absl::c_contains`, which provides a better name to the operation, but this is blocked by https://github.com/bazelbuild/bazel/issues/22719.
* Use the early-return pattern where possible to decrease nesting.
* Use references to remove unnecessary copies.
* Don't store unnecessary variables in the class. This decreases object size.
* Remove using-declarations for names in `std::`, which is discouraged.
PiperOrigin-RevId: 661149102
Change-Id: I2ab8fc617e1cec9f6bb81c6a3d5bbb883164f299
diff --git a/src/main/cpp/BUILD b/src/main/cpp/BUILD
index efedce5..f2ae66d 100644
--- a/src/main/cpp/BUILD
+++ b/src/main/cpp/BUILD
@@ -242,6 +242,13 @@
":workspace_layout",
"//src/main/cpp/util",
"//src/main/cpp/util:logging",
+ "@abseil-cpp//absl/algorithm:container",
+ "@abseil-cpp//absl/container:flat_hash_map",
+ "@abseil-cpp//absl/memory",
+ "@abseil-cpp//absl/strings",
+ "@abseil-cpp//absl/strings:str_format",
+ "@abseil-cpp//absl/strings:string_view",
+ "@abseil-cpp//absl/types:span",
],
)
diff --git a/src/main/cpp/rc_file.cc b/src/main/cpp/rc_file.cc
index 01016e0..d0d5f0a 100644
--- a/src/main/cpp/rc_file.cc
+++ b/src/main/cpp/rc_file.cc
@@ -14,8 +14,10 @@
#include "src/main/cpp/rc_file.h"
-#include <algorithm>
+#include <memory>
+#include <string>
#include <utility>
+#include <vector>
#include "src/main/cpp/blaze_util.h"
#include "src/main/cpp/blaze_util_platform.h"
@@ -23,47 +25,47 @@
#include "src/main/cpp/util/logging.h"
#include "src/main/cpp/util/strings.h"
#include "src/main/cpp/workspace_layout.h"
+#include "absl/algorithm/container.h"
+#include "absl/memory/memory.h"
+#include "absl/strings/match.h"
+#include "absl/strings/str_cat.h"
+#include "absl/strings/str_format.h"
+#include "absl/strings/str_split.h"
+#include "absl/strings/string_view.h"
+#include "absl/types/span.h"
namespace blaze {
-using std::deque;
-using std::string;
-using std::vector;
-
-static constexpr const char* kCommandImport = "import";
-static constexpr const char* kCommandTryImport = "try-import";
-
-RcFile::RcFile(string filename, const WorkspaceLayout* workspace_layout,
- string workspace)
- : filename_(std::move(filename)),
- workspace_layout_(workspace_layout),
- workspace_(std::move(workspace)) {}
+static constexpr absl::string_view kCommandImport = "import";
+static constexpr absl::string_view kCommandTryImport = "try-import";
/*static*/ std::unique_ptr<RcFile> RcFile::Parse(
- std::string filename, const WorkspaceLayout* workspace_layout,
- std::string workspace, ParseError* error, std::string* error_text) {
- std::unique_ptr<RcFile> rcfile(new RcFile(
- std::move(filename), workspace_layout, std::move(workspace)));
- deque<string> initial_import_stack = {rcfile->filename_};
- *error = rcfile->ParseFile(
- rcfile->filename_, &initial_import_stack, error_text);
+ const std::string& filename, const WorkspaceLayout* workspace_layout,
+ const std::string& workspace, ParseError* error, std::string* error_text) {
+ auto rcfile = absl::WrapUnique(new RcFile());
+ std::vector<std::string> initial_import_stack = {filename};
+ *error = rcfile->ParseFile(filename, workspace, *workspace_layout,
+ initial_import_stack, error_text);
return (*error == ParseError::NONE) ? std::move(rcfile) : nullptr;
}
-RcFile::ParseError RcFile::ParseFile(const string& filename,
- deque<string>* import_stack,
- string* error_text) {
+RcFile::ParseError RcFile::ParseFile(const std::string& filename,
+ const std::string& workspace,
+ const WorkspaceLayout& workspace_layout,
+ std::vector<std::string>& import_stack,
+ std::string* error_text) {
BAZEL_LOG(INFO) << "Parsing the RcFile " << filename;
- string contents;
- string error_message;
- if (!blaze_util::ReadFile(filename, &contents, &error_message)) {
- blaze_util::StringPrintf(error_text,
- "Unexpected error reading config file '%s': %s",
- filename.c_str(), error_message.c_str());
+ std::string contents;
+ if (std::string error_msg;
+ !blaze_util::ReadFile(filename, &contents, &error_msg)) {
+ *error_text = absl::StrFormat(
+ "Unexpected error reading config file '%s': %s", filename, error_msg);
return ParseError::UNREADABLE_FILE;
}
const std::string canonical_filename =
blaze_util::MakeCanonical(filename.c_str());
+ const absl::string_view workspace_prefix(
+ workspace_layout.WorkspacePrefix, workspace_layout.WorkspacePrefixLength);
int rcfile_index = canonical_rcfile_paths_.size();
canonical_rcfile_paths_.push_back(canonical_filename);
@@ -72,16 +74,14 @@
blaze_util::Replace("\\\r\n", "", &contents);
blaze_util::Replace("\\\n", "", &contents);
- vector<string> lines = blaze_util::Split(contents, '\n');
- for (string& line : lines) {
+ std::vector<std::string> lines = absl::StrSplit(contents, '\n');
+ for (std::string& line : lines) {
blaze_util::StripWhitespace(&line);
// Check for an empty line.
- if (line.empty()) {
- continue;
- }
+ if (line.empty()) continue;
- vector<string> words;
+ std::vector<std::string> words;
// This will treat "#" as a comment, and properly
// quote single and double quotes, and treat '\'
@@ -90,64 +90,68 @@
// dangling backslash escapes and missing end-quotes.
blaze_util::Tokenize(line, '#', &words);
- if (words.empty()) {
- // Could happen if line starts with "#"
+ // Could happen if line starts with "#"
+ if (words.empty()) continue;
+
+ const absl::string_view command = words[0];
+ if (command != kCommandImport && command != kCommandTryImport) {
+ for (absl::string_view word : absl::MakeConstSpan(words).subspan(1)) {
+ options_[command].push_back({std::string(word), rcfile_index});
+ }
continue;
}
- string command = words[0];
+ if (words.size() != 2) {
+ *error_text = absl::StrFormat(
+ "Invalid import declaration in config file '%s': '%s'",
+ canonical_filename, line);
+ return ParseError::INVALID_FORMAT;
+ }
- if (command == kCommandImport || command == kCommandTryImport) {
- if (words.size() != 2 ||
- (words[1].compare(0, workspace_layout_->WorkspacePrefixLength,
- workspace_layout_->WorkspacePrefix) == 0 &&
- !workspace_layout_->WorkspaceRelativizeRcFilePath(workspace_,
- &words[1]) &&
- command == kCommandImport)) {
- blaze_util::StringPrintf(
- error_text,
- "Invalid import declaration in config file '%s': '%s'"
+ std::string& import_filename = words[1];
+ if (absl::StartsWith(import_filename, workspace_prefix)) {
+ const bool could_relativize =
+ workspace_layout.WorkspaceRelativizeRcFilePath(workspace,
+ &import_filename);
+ if (!could_relativize && command == kCommandImport) {
+ *error_text = absl::StrFormat(
+ "Nonexistant path in import declaration in config file '%s': '%s'"
" (are you in your source checkout/WORKSPACE?)",
- canonical_filename.c_str(), line.c_str());
+ canonical_filename, line);
return ParseError::INVALID_FORMAT;
}
- if (std::find(import_stack->begin(), import_stack->end(), words[1]) !=
- import_stack->end()) {
- string loop;
- for (const string& imported_rc : *import_stack) {
- loop += " " + imported_rc + "\n";
- }
- loop += " " + words[1] + "\n"; // Include the loop.
- blaze_util::StringPrintf(error_text,
- "Import loop detected:\n%s", loop.c_str());
- return ParseError::IMPORT_LOOP;
- }
+ }
- import_stack->push_back(words[1]);
- ParseError parse_error = ParseFile(words[1], import_stack, error_text);
- if (parse_error != ParseError::NONE) {
- if (parse_error == ParseError::UNREADABLE_FILE &&
- command == kCommandTryImport) {
- // For try-import, we ignore it if we couldn't find a file.
- BAZEL_LOG(INFO) << "Skipped optional import of " << words[1]
- << ", the specified rc file either does not exist or "
- "is not readable.";
- *error_text = "";
- } else {
- // Files that are there but are malformed or introduce a loop are
- // still a problem, though, so perpetuate those errors as we would
- // for a normal import statement.
- return parse_error;
- }
+ if (absl::c_linear_search(import_stack, import_filename)) {
+ std::string loop;
+ for (const std::string& imported_rc : import_stack) {
+ absl::StrAppend(&loop, " ", imported_rc, "\n");
}
- import_stack->pop_back();
- } else {
- auto words_it = words.begin();
- words_it++; // Advance past command.
- for (; words_it != words.end(); words_it++) {
- options_[command].push_back({*words_it, rcfile_index});
+ absl::StrAppend(&loop, " ", import_filename, "\n"); // Include the loop.
+ *error_text = absl::StrCat("Import loop detected:\n", loop);
+ return ParseError::IMPORT_LOOP;
+ }
+
+ import_stack.push_back(import_filename);
+ if (ParseError parse_error =
+ ParseFile(import_filename, workspace, workspace_layout,
+ import_stack, error_text);
+ parse_error != ParseError::NONE) {
+ if (parse_error == ParseError::UNREADABLE_FILE &&
+ command == kCommandTryImport) {
+ // For try-import, we ignore it if we couldn't find a file.
+ BAZEL_LOG(INFO) << "Skipped optional import of " << import_filename
+ << ", the specified rc file either does not exist or "
+ "is not readable.";
+ *error_text = "";
+ } else {
+ // Files that are there but are malformed or introduce a loop are
+ // still a problem, though, so perpetuate those errors as we would
+ // for a normal import statement.
+ return parse_error;
}
}
+ import_stack.pop_back();
}
return ParseError::NONE;
diff --git a/src/main/cpp/rc_file.h b/src/main/cpp/rc_file.h
index 91bb119..88af39a 100644
--- a/src/main/cpp/rc_file.h
+++ b/src/main/cpp/rc_file.h
@@ -14,13 +14,12 @@
#ifndef BAZEL_SRC_MAIN_CPP_RC_FILE_H_
#define BAZEL_SRC_MAIN_CPP_RC_FILE_H_
-#include <deque>
#include <memory>
#include <string>
-#include <unordered_map>
#include <vector>
#include "src/main/cpp/workspace_layout.h"
+#include "absl/container/flat_hash_map.h"
namespace blaze {
@@ -39,8 +38,8 @@
// error and error text on failure.
enum class ParseError { NONE, UNREADABLE_FILE, INVALID_FORMAT, IMPORT_LOOP };
static std::unique_ptr<RcFile> Parse(
- std::string filename, const WorkspaceLayout* workspace_layout,
- std::string workspace, ParseError* error, std::string* error_text);
+ const std::string& filename, const WorkspaceLayout* workspace_layout,
+ const std::string& workspace, ParseError* error, std::string* error_text);
// Movable and copyable.
RcFile(const RcFile&) = default;
@@ -54,24 +53,19 @@
}
// Command -> all options for that command (in order of appearance).
- using OptionMap = std::unordered_map<std::string, std::vector<RcOption>>;
+ using OptionMap = absl::flat_hash_map<std::string, std::vector<RcOption>>;
const OptionMap& options() const { return options_; }
private:
- RcFile(std::string filename, const WorkspaceLayout* workspace_layout,
- std::string workspace);
+ RcFile() = default;
// Recursive call to parse a file and its imports.
ParseError ParseFile(const std::string& filename,
- std::deque<std::string>* import_stack,
+ const std::string& workspace,
+ const WorkspaceLayout& workspace_layout,
+ std::vector<std::string>& import_stack,
std::string* error_text);
- std::string filename_;
-
- // Workspace definition.
- const WorkspaceLayout* workspace_layout_;
- std::string workspace_;
-
// Full closure of rcfile paths imported from this file (including itself).
// These are all canonical paths, created with blaze_util::MakeCanonical.
// This also means all of these paths should exist.
diff --git a/src/test/cpp/rc_options_test.cc b/src/test/cpp/rc_options_test.cc
index 0a1bf28..030fb50 100644
--- a/src/test/cpp/rc_options_test.cc
+++ b/src/test/cpp/rc_options_test.cc
@@ -12,23 +12,25 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+#include <cstddef>
+#include <memory>
+#include <string>
#include <vector>
-#include "src/main/cpp/blaze_util.h"
#include "src/main/cpp/blaze_util_platform.h"
#include "src/main/cpp/option_processor.h"
#include "src/main/cpp/util/file.h"
#include "src/main/cpp/util/file_platform.h"
#include "src/main/cpp/util/path.h"
-#include "src/main/cpp/util/strings.h"
#include "src/main/cpp/workspace_layout.h"
#include "googlemock/include/gmock/gmock.h"
#include "googletest/include/gtest/gtest.h"
+#include "absl/container/flat_hash_map.h"
+#include "src/main/cpp/rc_file.h"
namespace blaze {
-using std::string;
-using std::unordered_map;
-using std::vector;
+namespace {
+
using ::testing::MatchesRegex;
#if _WIN32
@@ -46,10 +48,10 @@
workspace_layout_() {
}
- const string test_file_dir_;
+ const std::string test_file_dir_;
const WorkspaceLayout workspace_layout_;
- void WriteRc(const string& filename, const string& contents) {
+ void WriteRc(const std::string& filename, const std::string& contents) {
bool success_mkdir = blaze_util::MakeDirectories(test_file_dir_, 0755);
ASSERT_TRUE(success_mkdir) << "Failed to mkdir -p " << test_file_dir_;
bool success_write = blaze_util::WriteFile(
@@ -57,7 +59,7 @@
ASSERT_TRUE(success_write) << "Failed to write " << filename;
}
- std::unique_ptr<RcFile> Parse(const string& filename,
+ std::unique_ptr<RcFile> Parse(const std::string& filename,
RcFile::ParseError* error,
std::string* error_text) {
return RcFile::Parse(
@@ -70,10 +72,11 @@
}
void SuccessfullyParseRcWithExpectedArgs(
- const string& filename,
- const unordered_map<string, vector<string>>& expected_args_map) {
+ const std::string& filename,
+ const absl::flat_hash_map<std::string, std::vector<std::string>>&
+ expected_args_map) {
RcFile::ParseError error;
- string error_text;
+ std::string error_text;
std::unique_ptr<RcFile> rc = Parse(filename, &error, &error_text);
EXPECT_EQ(error_text, "");
ASSERT_EQ(error, RcFile::ParseError::NONE);
@@ -83,11 +86,11 @@
// correct order. Note that this is not just an exercise in rewriting map
// equality - the results have type RcOption, and the expected values
// are just strings. This is ignoring the source_path for convenience.
- const RcFile::OptionMap& result = rc->options();
+ RcFile::OptionMap result = rc->options();
ASSERT_EQ(expected_args_map.size(), result.size());
for (const auto& command_args_pair : expected_args_map) {
- const string& expected_command = command_args_pair.first;
- const vector<string>& expected_args = command_args_pair.second;
+ const std::string& expected_command = command_args_pair.first;
+ const std::vector<std::string>& expected_args = command_args_pair.second;
const auto result_args_iter = result.find(expected_command);
ASSERT_NE(result_args_iter, rc->options().end());
const std::vector<RcOption>& result_args = result_args_iter->second;
@@ -102,21 +105,21 @@
TEST_F(RcOptionsTest, Empty) {
WriteRc("empty.bazelrc",
"");
- unordered_map<string, vector<string>> no_expected_args;
+ absl::flat_hash_map<std::string, std::vector<std::string>> no_expected_args;
SuccessfullyParseRcWithExpectedArgs("empty.bazelrc", no_expected_args);
}
TEST_F(RcOptionsTest, Whitespace) {
WriteRc("whitespace.bazelrc",
" \n\t ");
- unordered_map<string, vector<string>> no_expected_args;
+ absl::flat_hash_map<std::string, std::vector<std::string>> no_expected_args;
SuccessfullyParseRcWithExpectedArgs("whitespace.bazelrc", no_expected_args);
}
TEST_F(RcOptionsTest, CommentedStartup) {
WriteRc("commented_startup.bazelrc",
"# startup foo");
- unordered_map<string, vector<string>> no_expected_args;
+ absl::flat_hash_map<std::string, std::vector<std::string>> no_expected_args;
SuccessfullyParseRcWithExpectedArgs("commented_startup.bazelrc",
no_expected_args);
}
@@ -124,7 +127,7 @@
TEST_F(RcOptionsTest, EmptyStartupLine) {
WriteRc("empty_startup_line.bazelrc",
"startup");
- unordered_map<string, vector<string>> no_expected_args;
+ absl::flat_hash_map<std::string, std::vector<std::string>> no_expected_args;
SuccessfullyParseRcWithExpectedArgs("empty_startup_line.bazelrc",
no_expected_args);
}
@@ -132,7 +135,7 @@
TEST_F(RcOptionsTest, StartupWithOnlyCommentedArg) {
WriteRc("startup_with_comment.bazelrc",
"startup # bar");
- unordered_map<string, vector<string>> no_expected_args;
+ absl::flat_hash_map<std::string, std::vector<std::string>> no_expected_args;
SuccessfullyParseRcWithExpectedArgs("startup_with_comment.bazelrc",
no_expected_args);
}
@@ -356,7 +359,7 @@
"import %workspace%/import_cycle_1.bazelrc");
RcFile::ParseError error;
- string error_text;
+ std::string error_text;
std::unique_ptr<RcFile> rc =
Parse("import_cycle_1.bazelrc", &error, &error_text);
EXPECT_EQ(error, RcFile::ParseError::IMPORT_LOOP);
@@ -383,7 +386,7 @@
"import %workspace%/import_cycle_1.bazelrc");
RcFile::ParseError error;
- string error_text;
+ std::string error_text;
std::unique_ptr<RcFile> rc =
Parse("chain_to_cycle_1.bazelrc", &error, &error_text);
EXPECT_EQ(error, RcFile::ParseError::IMPORT_LOOP);
@@ -401,7 +404,7 @@
TEST_F(RcOptionsTest, FileDoesNotExist) {
RcFile::ParseError error;
- string error_text;
+ std::string error_text;
std::unique_ptr<RcFile> rc = Parse("not_a_file.bazelrc", &error, &error_text);
EXPECT_EQ(error, RcFile::ParseError::UNREADABLE_FILE);
ASSERT_THAT(
@@ -418,7 +421,7 @@
"import somefile");
RcFile::ParseError error;
- string error_text;
+ std::string error_text;
std::unique_ptr<RcFile> rc =
Parse("import_fake_file.bazelrc", &error, &error_text);
EXPECT_EQ(error, RcFile::ParseError::UNREADABLE_FILE);
@@ -437,7 +440,7 @@
TEST_F(RcOptionsTest, TryImportedFileDoesNotExist) {
WriteRc("try_import_fake_file.bazelrc", "try-import somefile");
- unordered_map<string, vector<string>> no_expected_args;
+ absl::flat_hash_map<std::string, std::vector<std::string>> no_expected_args;
SuccessfullyParseRcWithExpectedArgs("try_import_fake_file.bazelrc",
no_expected_args);
}
@@ -447,28 +450,25 @@
"import somefile bar");
RcFile::ParseError error;
- string error_text;
+ std::string error_text;
std::unique_ptr<RcFile> rc = Parse("bad_import.bazelrc", &error, &error_text);
EXPECT_EQ(error, RcFile::ParseError::INVALID_FORMAT);
- ASSERT_THAT(
- error_text,
- MatchesRegex("Invalid import declaration in config file "
- "'.*bad_import.bazelrc': 'import somefile bar' \\(are you "
- "in your source checkout/WORKSPACE\\?\\)"));
+ ASSERT_THAT(error_text,
+ MatchesRegex("Invalid import declaration in config file "
+ "'.*bad_import.bazelrc': 'import somefile bar'"));
}
TEST_F(RcOptionsTest, TryImportHasTooManyArgs) {
WriteRc("bad_import.bazelrc", "try-import somefile bar");
RcFile::ParseError error;
- string error_text;
+ std::string error_text;
std::unique_ptr<RcFile> rc = Parse("bad_import.bazelrc", &error, &error_text);
EXPECT_EQ(error, RcFile::ParseError::INVALID_FORMAT);
ASSERT_THAT(
error_text,
MatchesRegex("Invalid import declaration in config file "
- "'.*bad_import.bazelrc': 'try-import somefile bar' \\(are "
- "you in your source checkout/WORKSPACE\\?\\)"));
+ "'.*bad_import.bazelrc': 'try-import somefile bar'"));
}
// TODO(b/34811299) The tests below identify ways that '\' used as a line
@@ -522,5 +522,5 @@
{{"startup", {"foo", " "}}, {"bar", {"baz"}}});
}
+} // namespace
} // namespace blaze
-
