Simplify memory management for RcFile.
RcFile uses pointers to avoid copying the source paths, which leads to some very tricky gotchas in the code. This change converts the pointers to integer indices, which are stable and easier to work with. It adds a tiny bit of overhead when using those indices, but makes the entire class much easier to work with.
Note: This change makes it theoretically possible to replace the unique_ptr in RcFile::Parse with a simpler type like std::optional, but that requires C++17 which Bazel's build doesn't currently support. There's a C++11 compatible version in absl, but that requires bringing in that extra dependency which would need to be done separately. For now, I couldn't think of a better way to handle it than leaving it a unique_ptr.
RELNOTES: none
PiperOrigin-RevId: 329574479
diff --git a/src/test/cpp/rc_file_test.cc b/src/test/cpp/rc_file_test.cc
index 6751768..70bf7dd 100644
--- a/src/test/cpp/rc_file_test.cc
+++ b/src/test/cpp/rc_file_test.cc
@@ -31,8 +31,11 @@
namespace blaze {
using ::testing::ContainsRegex;
+using ::testing::ElementsAre;
using ::testing::HasSubstr;
+using ::testing::IsEmpty;
using ::testing::MatchesRegex;
+using ::testing::Pointee;
#if defined(_WIN32) || defined(__CYGWIN__)
constexpr const char* kNullDevice = "NUL";
@@ -40,6 +43,12 @@
constexpr const char* kNullDevice = "/dev/null";
#endif
+// Matches an RcFile's canonical source paths list.
+MATCHER_P(CanonicalSourcePathsAre, paths_matcher, "") {
+ return ExplainMatchResult(ElementsAre(paths_matcher),
+ arg.canonical_source_paths(), result_listener);
+}
+
class RcFileTest : public ::testing::Test {
protected:
RcFileTest()
@@ -172,13 +181,9 @@
// There should be 2 rc files: the system one and the workspace one. --bazelrc
// is not passed and therefore is not relevant.
- ASSERT_EQ(2, parsed_rcs.size());
- const std::deque<std::string> expected_system_rc_que = {system_rc};
- const std::deque<std::string> expected_workspace_rc_que = {workspace_rc};
- EXPECT_EQ(expected_system_rc_que,
- parsed_rcs[0].get()->canonical_source_paths());
- EXPECT_EQ(expected_workspace_rc_que,
- parsed_rcs[1].get()->canonical_source_paths());
+ EXPECT_THAT(parsed_rcs,
+ ElementsAre(Pointee(CanonicalSourcePathsAre(system_rc)),
+ Pointee(CanonicalSourcePathsAre(workspace_rc))));
}
TEST_F(GetRcFileTest, GetRcFilesRespectsNoSystemRc) {
@@ -197,10 +202,8 @@
EXPECT_EQ(blaze_exit_code::SUCCESS, exit_code);
EXPECT_EQ("check that this string is not modified", error);
- ASSERT_EQ(1, parsed_rcs.size());
- const std::deque<std::string> expected_workspace_rc_que = {workspace_rc};
- EXPECT_EQ(expected_workspace_rc_que,
- parsed_rcs[0].get()->canonical_source_paths());
+ EXPECT_THAT(parsed_rcs,
+ ElementsAre(Pointee(CanonicalSourcePathsAre(workspace_rc))));
}
TEST_F(GetRcFileTest, GetRcFilesRespectsNoWorkspaceRc) {
@@ -219,10 +222,8 @@
EXPECT_EQ(blaze_exit_code::SUCCESS, exit_code);
EXPECT_EQ("check that this string is not modified", error);
- ASSERT_EQ(1, parsed_rcs.size());
- const std::deque<std::string> expected_system_rc_que = {system_rc};
- EXPECT_EQ(expected_system_rc_que,
- parsed_rcs[0].get()->canonical_source_paths());
+ EXPECT_THAT(parsed_rcs,
+ ElementsAre(Pointee(CanonicalSourcePathsAre(system_rc))));
}
TEST_F(GetRcFileTest, GetRcFilesRespectsNoWorkspaceRcAndNoSystemCombined) {
@@ -241,7 +242,7 @@
EXPECT_EQ(blaze_exit_code::SUCCESS, exit_code);
EXPECT_EQ("check that this string is not modified", error);
- ASSERT_EQ(0, parsed_rcs.size());
+ EXPECT_THAT(parsed_rcs, IsEmpty());
}
TEST_F(GetRcFileTest, GetRcFilesWarnsAboutIgnoredMasterRcFiles) {
@@ -323,8 +324,8 @@
// Because of the variety of path representations in windows, this
// equality test does not attempt to check the entire path.
ASSERT_EQ(1, parsed_rcs.size());
- ASSERT_EQ(1, parsed_rcs[0].get()->canonical_source_paths().size());
- EXPECT_THAT(parsed_rcs[0].get()->canonical_source_paths().front(),
+ ASSERT_EQ(1, parsed_rcs[0]->canonical_source_paths().size());
+ EXPECT_THAT(parsed_rcs[0]->canonical_source_paths()[0],
HasSubstr("mybazelrc"));
}
@@ -343,9 +344,8 @@
EXPECT_EQ(blaze_exit_code::SUCCESS, exit_code);
EXPECT_EQ("check that this string is not modified", error);
// but it does technically count as a file
- ASSERT_EQ(1, parsed_rcs.size());
- const std::deque<std::string> expected_rc_que = {kNullDevice};
- EXPECT_EQ(expected_rc_que, parsed_rcs[0].get()->canonical_source_paths());
+ EXPECT_THAT(parsed_rcs,
+ ElementsAre(Pointee(CanonicalSourcePathsAre(kNullDevice))));
}
class ParseOptionsTest : public RcFileTest {
