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.