Follow-up fixes to filesystem code (#5949)

Tidies up extraneous move, unnecessary function style type cast, and
simplifies the temporary directory string construction. These were
noticed during another PR review.

Also corrects support for older glibc versions, including the
GNU-specific quirks of `strerror_r`. Restricts the fancier formatting
with the name of the error number to when a recent glibc is available.

Lastly, filters the benchmarks in the benchmark test down to smaller
ones to avoid test timeout flakiness.

common/BUILD

@@ -261,7 +261,12 @@ sh_test(
     name = "filesystem_benchmark_test",
     size = "small",
     srcs = [":filesystem_benchmark"],
-    args = ["--benchmark_min_time=1x"],
+    args = [
+        "--benchmark_min_time=1x",
+        # Restrict the sizes to 4-digit ones or smaller to keep test times low.
+        # The `$$` is repeated for Bazel escaping of `$`.
+        "--benchmark_filter=^[^/]+(/[0-9]{1,4}(/[0-9]+)?)?/real_time$$",
+    ],
 )
 
 cc_library(

common/filesystem.cpp

@@ -15,17 +15,27 @@ namespace Carbon::Filesystem {
 // Render an error number from `errno` to the provided stream using the richest
 // rendering available on the platform.
 static auto PrintErrorNumber(llvm::raw_ostream& out, int errnum) -> void {
-#ifdef _GNU_SOURCE
-  // Use GNU-specific routines to compute the error name and description.
+#if defined(_GNU_SOURCE) && \
+    (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 32))
+  // For sufficiently recent glibc versions, use GNU-specific routines to
+  // compute the error name and description.
   llvm::StringRef name = strerrordesc_np(errnum);
   llvm::StringRef desc = strerrorname_np(errnum);
 
   out << llvm::formatv("{0}: {1}", name, desc);
-#elif defined(__APPLE__) || defined(_POSIX_SOURCE)
+#elif defined(__APPLE__) || defined(_GNU_SOURCE) || defined(_POSIX_SOURCE)
+  // Broadly portable fallback for Unix-like systems.
   char buffer[4096];
+#ifdef _GNU_SOURCE
+  const char* str = strerror_r(errnum, buffer, sizeof(buffer));
+  // The GNU version doesn't report a meta-error.
+  int meta_error = 0;
+#else
   int meta_error = strerror_r(errnum, buffer, sizeof(buffer));
+  const char* str = buffer;
+#endif
   if (meta_error == 0) {
-    out << llvm::formatv("errno {0}: {1}", errnum, llvm::StringRef(buffer));
+    out << llvm::formatv("errno {0}: {1}", errnum, llvm::StringRef(str));
   } else {
     out << llvm::formatv(
         "error number {0}; encountered meta-error number {1} while rendering "
@@ -479,7 +489,7 @@ auto MakeTmpDir() -> ErrorOr<RemovingDir, Error> {
     if (tmpdir_env_cstr == nullptr) {
       continue;
     }
-    std::filesystem::path tmpdir_env = std::string(tmpdir_env_cstr);
+    std::filesystem::path tmpdir_env = tmpdir_env_cstr;
     if (!tmpdir_env.is_absolute()) {
       continue;
     }
@@ -488,10 +498,8 @@ auto MakeTmpDir() -> ErrorOr<RemovingDir, Error> {
   }
 
   std::filesystem::path target = BuildData::BuildTarget.str();
-  std::string dir_name = target.filename().native();
-  dir_name += ".XXXXXX";
-
-  tmpdir_path /= dir_name;
+  tmpdir_path /= target.filename();
+  tmpdir_path += ".XXXXXX";
 
   std::string tmpdir_path_buffer = tmpdir_path.native();
   char* result = mkdtemp(tmpdir_path_buffer.data());

common/filesystem_benchmark.cpp

@@ -71,7 +71,7 @@ struct BenchContext {
   std::array<std::filesystem::path, NumFiles> file_paths;
   std::array<std::filesystem::path, NumFiles> missing_paths;
 
-  BenchContext() : tmpdir(std::move(*MakeTmpDir())) {
+  BenchContext() : tmpdir(*MakeTmpDir()) {
     for (int i : llvm::seq(NumFiles)) {
       file_paths[i] = llvm::formatv("file_{0}", i).str();
       auto result = tmpdir.WriteFileFromString(file_paths[i], Text);

common/filesystem_test.cpp

@@ -45,11 +45,11 @@ TEST_F(FilesystemTest, CreateOpenCloseAndUnlink) {
   auto unlink_result = dir_.Unlink("test");
   ASSERT_FALSE(unlink_result.ok());
   EXPECT_TRUE(unlink_result.error().no_entity());
-#ifdef _GNU_SOURCE
+#if defined(_GNU_SOURCE) && \
+    (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 32))
   EXPECT_THAT(unlink_result, IsError(HasSubstr("ENOENT")));
-#elif defined(__APPLE__) || defined(_POSIX_SOURCE)
-  EXPECT_THAT(unlink_result, IsError(HasSubstr("No such file")));
 #endif
+  EXPECT_THAT(unlink_result, IsError(HasSubstr("No such file")));
 
   auto f = dir_.OpenWriteOnly("test", CreationOptions::CreateNew);
   ASSERT_THAT(f, IsSuccess(_));
@@ -59,11 +59,11 @@ TEST_F(FilesystemTest, CreateOpenCloseAndUnlink) {
   f = dir_.OpenWriteOnly("test", CreationOptions::CreateNew);
   ASSERT_FALSE(f.ok());
   EXPECT_TRUE(f.error().already_exists());
-#ifdef _GNU_SOURCE
+#if defined(_GNU_SOURCE) && \
+    (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 32))
   EXPECT_THAT(f, IsError(HasSubstr("EEXIST")));
-#elif defined(__APPLE__) || defined(_POSIX_SOURCE)
-  EXPECT_THAT(f, IsError(HasSubstr("File exists")));
 #endif
+  EXPECT_THAT(f, IsError(HasSubstr("File exists")));
 
   f = dir_.OpenWriteOnly("test");
   ASSERT_THAT(f, IsSuccess(_));
@@ -81,11 +81,11 @@ TEST_F(FilesystemTest, CreateOpenCloseAndUnlink) {
   f = dir_.OpenWriteOnly("test");
   EXPECT_FALSE(f.ok());
   EXPECT_TRUE(f.error().no_entity());
-#ifdef _GNU_SOURCE
+#if defined(_GNU_SOURCE) && \
+    (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 32))
   EXPECT_THAT(f, IsError(HasSubstr("ENOENT")));
-#elif defined(__APPLE__) || defined(_POSIX_SOURCE)
-  EXPECT_THAT(f, IsError(HasSubstr("No such file")));
 #endif
+  EXPECT_THAT(f, IsError(HasSubstr("No such file")));
 
   f = dir_.OpenWriteOnly("test", CreationOptions::OpenAlways);
   ASSERT_THAT(f, IsSuccess(_));