[Support] sys::fs::directory_entry includes the file_type.

[sammccall]

Summary

This is available on most platforms (Linux/Mac/Win/BSD) with no extra syscalls.
On other platforms (e.g. Solaris) we stat() if this information is requested.

This will allow switching clang's VFS to efficiently expose (path, type) when
traversing a directory. Currently it exposes an entire Status, but does so by
calling fs::status() on all platforms.
Almost all callers only need the path, and all callers only need (path, type).

[bkramer]

lg

lib/Support/Unix/Path.inc

708 ↗

(On Diff #164828)

Funky formatting

[kristina]

Would it be okay to use IT instead of it, as you're already changing those parts, since that follows the general variable naming convention? If possible can same be done for cur_dir and alike (ie. CurDir)? Sorry to butt in like that but there's a lot of style inconsistencies, may be a good opportunity to fix them instead of adding more code that uses the same style violations.

(To elaborate I'm mostly referring to the naming conventions used for locals, which is currently massively inconsistent)

[kristina]

Style consistency.

Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).

If possible please fix existing naming violations in functions which this patch modifies, as well as newly introduced violations.

lib/Support/Unix/Path.inc

714 ↗

(On Diff #164828)

cur_dir should be CurDir, it should be uppercase IT in signature and throughout the function.

718 ↗

(On Diff #164828)

name to Name if possible.

lib/Support/Windows/Path.inc

951 ↗

(On Diff #164828)

IT instead of it. 'EC' instead of 'ec'.

954 ↗

(On Diff #164828)

Same as my previous comment, IT instead of it.

[sammccall]

Fix formatting glitch.
Update names of locals in touched functions to match style guide.

[sammccall]

In D51918#1232281, @kristina wrote:

Style consistency.

Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).

If possible please fix existing naming violations in functions which this patch modifies, as well as newly introduced violations.

Yep. I was aiming for consistency with the surrounding code, since the whole file (including its public APIs) are using an old style.
I've now instead changed all the locals in affected functions to use UpperCamelCase, as well as a couple of static helper functions. I haven't changed public function names etc, or the other functions in the same file.

Because I can't build the code in Windows/Path.inc, I'm concerned all this renaming may have introduced errors, but that's what buildbots are for I guess.

[kristina]

Thank you!

[kristina]

@sammccall I'll run the build on a Windows machine if that gives you any peace of mind, just preparing to.

[kristina]

Windows build failure after grafting the patch:

Severity	Code	Description	Project	File	Line	Suppression State
Error	C2065	'FindData': undeclared identifier (compiling source file Q:\SourceCache\llvm-trunk-8.0\lib\Support\Path.cpp)	LLVMSupport	q:\sourcecache\llvm-trunk-8.0\lib\support\Windows\Path.inc	956	
Error	C2227	left of '->dwFileAttributes' must point to class/struct/union/generic type (compiling source file Q:\SourceCache\llvm-trunk-8.0\lib\Support\Path.cpp)	LLVMSupport	q:\sourcecache\llvm-trunk-8.0\lib\support\Windows\Path.inc	956	
Error	C2440	'<function-style-cast>': cannot convert from 'initializer list' to 'llvm::sys::fs::directory_entry' (compiling source file Q:\SourceCache\llvm-trunk-8.0\lib\Support\Path.cpp)	LLVMSupport	q:\sourcecache\llvm-trunk-8.0\lib\support\Windows\Path.inc	957	
Error	C2819	type '_WIN32_FIND_DATAW' does not have an overloaded member 'operator ->' (compiling source file Q:\SourceCache\llvm-trunk-8.0\lib\Support\Path.cpp)	LLVMSupport	q:\sourcecache\llvm-trunk-8.0\lib\support\Windows\Path.inc	995	
Error	C2232	'->_WIN32_FIND_DATAW::dwFileAttributes': left operand has 'struct' type, use '.' (compiling source file Q:\SourceCache\llvm-trunk-8.0\lib\Support\Path.cpp)	LLVMSupport	q:\sourcecache\llvm-trunk-8.0\lib\support\Windows\Path.inc	995	
Error	C2664	'void llvm::sys::fs::directory_entry::replace_filename(const llvm::Twine &,llvm::sys::fs::file_type,llvm::sys::fs::basic_file_status)': cannot convert argument 2 from 'llvm::sys::fs::basic_file_status' to 'llvm::sys::fs::file_type' (compiling source file Q:\SourceCache\llvm-trunk-8.0\lib\Support\Path.cpp)	LLVMSupport	q:\sourcecache\llvm-trunk-8.0\lib\support\Windows\Path.inc	996

(Hey at least I have a Windows build environment now!)

[kristina]

Shall I just fix up the Windows stuff and add another diff if you don't mind?

I assume you meant FirstFind and not FileData in this call?

std::error_code detail::directory_iterator_construct(detail::DirIterState &IT,
                                                     StringRef Path,
                                                     bool FollowSymlinks) {
  // - snip -
  IT.CurrentEntry =
      directory_entry(DirectoryEntryPath, FollowSymlinks,
                      file_type_from_attrs(FindData->dwFileAttributes),
                      status_from_find_data(&FirstFind));

[sammccall]

Fix silly windows mistakes.

[sammccall]

In D51918#1232441, @kristina wrote:

Shall I just fix up the Windows stuff and add another diff if you don't mind?

Thanks very much! I'm more reliant on my IDE than I thought!

Please do upload a new version if it's not too much trouble (I added one that might work, but I can't verify)

[kristina]

In D51918#1232477, @sammccall wrote:

In D51918#1232441, @kristina wrote:

Shall I just fix up the Windows stuff and add another diff if you don't mind?

Thanks very much! I'm more reliant on my IDE than I thought!

Please do upload a new version if it's not too much trouble (I added one that might work, but I can't verify)

I have a working patch with further format fixes but I can't seem to be able to attach it to this revision. How would I do that?

PS Q:\BuildRoot\llvm-8.0-win32-oneshot\RelWithDebInfo\lib> Get-Item LLVMSu*
    Directory: Q:\BuildRoot\llvm-8.0-win32-oneshot\RelWithDebInfo\lib
Mode                LastWriteTime         Length Name
----                -------------         ------ ----
-a----       12/09/2018     14:14       17083534 LLVMSupport.lib

[sammccall]

If you're using arcanist then arc diff --update D51918 should work.
Otherwise I think you can use the "update diff" to manually upload a patch in phabricator.

Failing that, if it's just formatting changes you can land it, make sure to include this in the commit message:
Differential Revision: https://reviews.llvm.org/D51918

[kristina]

Alright, I'll land it myself, it's just minor style nitpicks but I know it compiles.

[jordan_rose]

llvm/trunk/lib/Support/Unix/Path.inc

704

Picking this up a year later by noticing that _DIRENT_HAVE_D_TYPE isn't actually defined in macOS's headers, which means it probably isn't in BSD either. Do you think just testing for DTTOIF would be sufficient?

[sammccall]

llvm/trunk/lib/Support/Unix/Path.inc

704

Oops, I didn't realize that while d_type is a BSD extension that GNU picked up, _DIRENT_HAVE_D_TYPE seems to be a GNU extension that BSD doesn't have :-\

Do you think just testing for DTTOIF would be sufficient?

That sounds plausible to me, there isn't much point defining DTTOIF if you don't have d_type to pass to it.

That sounds plausible to me. And the "good news" is that it would fail with a noisy compile error.
So I think the worst case is the patch you propose ends up getting reverted.

I guess the "proper" way to do this is to have CMake detect this and create a HAVE_DIRENT_D_TYPE macro in config.h. But I have no objection to trying the simple thing first.

llvm/trunk/include/llvm/Support/FileSystem.h

 /// program startup and the address of main itself. In extremis, this function
 /// may fail and return an empty path.
 std::string getMainExecutable(const char *argv0, void *MainExecAddr);
 
 /// @}
 /// @name Iterators
 /// @{
 
-/// directory_entry - A single entry in a directory. Caches the status either
-/// from the result of the iteration syscall, or the first time status is
-/// called.
+/// directory_entry - A single entry in a directory.
 class directory_entry {
+  // FIXME: different platforms make different information available "for free"
+  // when traversing a directory. The design of this class wraps most of the
+  // information in basic_file_status, so on platforms where we can't populate
+  // that whole structure, callers end up paying for a stat().
+  // std::filesystem::directory_entry may be a better model.
   std::string Path;
-  bool FollowSymlinks;
-  basic_file_status Status;
+  file_type Type;           // Most platforms can provide this.
+  bool FollowSymlinks;      // Affects the behavior of status().
+  basic_file_status Status; // If available.
 
 public:
-  explicit directory_entry(const Twine &path, bool follow_symlinks = true,
-                           basic_file_status st = basic_file_status())
-      : Path(path.str()), FollowSymlinks(follow_symlinks), Status(st) {}
+  explicit directory_entry(const Twine &Path, bool FollowSymlinks = true,
+                           file_type Type = file_type::type_unknown,
+                           basic_file_status Status = basic_file_status())
+      : Path(Path.str()), Type(Type), FollowSymlinks(FollowSymlinks),
+        Status(Status) {}
 
   directory_entry() = default;
 
-  void assign(const Twine &path, basic_file_status st = basic_file_status()) {
-    Path = path.str();
-    Status = st;
-  }
-
-  void replace_filename(const Twine &filename,
-                        basic_file_status st = basic_file_status());
+  void replace_filename(const Twine &Filename, file_type Type,
+                        basic_file_status Status = basic_file_status());
 
   const std::string &path() const { return Path; }
+  // Get basic information about entry file (a subset of fs::status()).
+  // On most platforms this is a stat() call.
+  // On windows the information was already retrieved from the directory.
   ErrorOr<basic_file_status> status() const;
-
-  bool operator==(const directory_entry& rhs) const { return Path == rhs.Path; }
-  bool operator!=(const directory_entry& rhs) const { return !(*this == rhs); }
-  bool operator< (const directory_entry& rhs) const;
-  bool operator<=(const directory_entry& rhs) const;
-  bool operator> (const directory_entry& rhs) const;
-  bool operator>=(const directory_entry& rhs) const;
+  // Get the type of this file.
+  // On most platforms (Linux/Mac/Windows/BSD), this was already retrieved.
+  // On some platforms (e.g. Solaris) this is a stat() call.
+  file_type type() const {
+    if (Type != file_type::type_unknown)
+      return Type;
+    auto S = status();
+    return S ? S->type() : file_type::type_unknown;
+  }
+
+  bool operator==(const directory_entry& RHS) const { return Path == RHS.Path; }
+  bool operator!=(const directory_entry& RHS) const { return !(*this == RHS); }
+  bool operator< (const directory_entry& RHS) const;
+  bool operator<=(const directory_entry& RHS) const;
+  bool operator> (const directory_entry& RHS) const;
+  bool operator>=(const directory_entry& RHS) const;
 };
 
 namespace detail {
 
   struct DirIterState;
 
   std::error_code directory_iterator_construct(DirIterState &, StringRef, bool);
   std::error_code directory_iterator_increment(DirIterState &);

llvm/trunk/lib/Support/Path.cpp

 std::error_code is_other(const Twine &Path, bool &Result) {
   file_status FileStatus;
   if (std::error_code EC = status(Path, FileStatus))
     return EC;
   Result = is_other(FileStatus);
   return std::error_code();
 }
 
-void directory_entry::replace_filename(const Twine &filename,
-                                       basic_file_status st) {
-  SmallString<128> path = path::parent_path(Path);
-  path::append(path, filename);
-  Path = path.str();
-  Status = st;
+void directory_entry::replace_filename(const Twine &Filename, file_type Type,
+                                       basic_file_status Status) {
+  SmallString<128> PathStr = path::parent_path(Path);
+  path::append(PathStr, Filename);
+  this->Path = PathStr.str();
+  this->Type = Type;
+  this->Status = Status;
 }
 
 ErrorOr<perms> getPermissions(const Twine &Path) {
   file_status Status;
   if (std::error_code EC = status(Path, Status))
     return EC;
 
   return Status.permissions();

llvm/trunk/lib/Support/Unix/Path.inc

static void expandTildeExpr(SmallVectorImpl<char> &Path) {
   }
 
   Storage = Remainder;
   Path.clear();
   Path.append(Entry->pw_dir, Entry->pw_dir + strlen(Entry->pw_dir));
   llvm::sys::path::append(Path, Storage);
 }
 
+static file_type typeForMode(mode_t Mode) {
+  if (S_ISDIR(Mode))
+    return file_type::directory_file;
+  else if (S_ISREG(Mode))
+    return file_type::regular_file;
+  else if (S_ISBLK(Mode))
+    return file_type::block_file;
+  else if (S_ISCHR(Mode))
+    return file_type::character_file;
+  else if (S_ISFIFO(Mode))
+    return file_type::fifo_file;
+  else if (S_ISSOCK(Mode))
+    return file_type::socket_file;
+  else if (S_ISLNK(Mode))
+    return file_type::symlink_file;
+  return file_type::type_unknown;
+}
+
 static std::error_code fillStatus(int StatRet, const struct stat &Status,
                                   file_status &Result) {
   if (StatRet != 0) {
-    std::error_code ec(errno, std::generic_category());
-    if (ec == errc::no_such_file_or_directory)
+    std::error_code EC(errno, std::generic_category());
+    if (EC == errc::no_such_file_or_directory)
       Result = file_status(file_type::file_not_found);
     else
       Result = file_status(file_type::status_error);
-    return ec;
+    return EC;
   }
 
-  file_type Type = file_type::type_unknown;
-
-  if (S_ISDIR(Status.st_mode))
-    Type = file_type::directory_file;
-  else if (S_ISREG(Status.st_mode))
-    Type = file_type::regular_file;
-  else if (S_ISBLK(Status.st_mode))
-    Type = file_type::block_file;
-  else if (S_ISCHR(Status.st_mode))
-    Type = file_type::character_file;
-  else if (S_ISFIFO(Status.st_mode))
-    Type = file_type::fifo_file;
-  else if (S_ISSOCK(Status.st_mode))
-    Type = file_type::socket_file;
-  else if (S_ISLNK(Status.st_mode))
-    Type = file_type::symlink_file;
-
   perms Perms = static_cast<perms>(Status.st_mode) & all_perms;
-  Result = file_status(Type, Perms, Status.st_dev, Status.st_nlink,
-                       Status.st_ino, Status.st_atime, Status.st_mtime,
-                       Status.st_uid, Status.st_gid, Status.st_size);
+  Result = file_status(typeForMode(Status.st_mode), Perms, Status.st_dev,
+                       Status.st_nlink, Status.st_ino, Status.st_atime,
+                       Status.st_mtime, Status.st_uid, Status.st_gid,
+                       Status.st_size);
 
   return std::error_code();
 }
 
 std::error_code status(const Twine &Path, file_status &Result, bool Follow) {
   SmallString<128> PathStorage;
   StringRef P = Path.toNullTerminatedStringRef(PathStorage);
 

 std::error_code detail::directory_iterator_destruct(detail::DirIterState &it) {
   if (it.IterationHandle)
     ::closedir(reinterpret_cast<DIR *>(it.IterationHandle));
   it.IterationHandle = 0;
   it.CurrentEntry = directory_entry();
   return std::error_code();
 }
 
-std::error_code detail::directory_iterator_increment(detail::DirIterState &it) {
+static file_type direntType(dirent* Entry) {
+  // Most platforms provide the file type in the dirent: Linux/BSD/Mac.
+  // The DTTOIF macro lets us reuse our status -> type conversion.
+#if defined(_DIRENT_HAVE_D_TYPE) && defined(DTTOIF)
+  return typeForMode(DTTOIF(Entry->d_type));
+#else
+  // Other platforms such as Solaris require a stat() to get the type.
+  return file_type::type_unknown;
+#endif
+}
+
+std::error_code detail::directory_iterator_increment(detail::DirIterState &It) {
   errno = 0;
-  dirent *cur_dir = ::readdir(reinterpret_cast<DIR *>(it.IterationHandle));
-  if (cur_dir == nullptr && errno != 0) {
+  dirent *CurDir = ::readdir(reinterpret_cast<DIR *>(It.IterationHandle));
+  if (CurDir == nullptr && errno != 0) {
     return std::error_code(errno, std::generic_category());
-  } else if (cur_dir != nullptr) {
-    StringRef name(cur_dir->d_name);
-    if ((name.size() == 1 && name[0] == '.') ||
-        (name.size() == 2 && name[0] == '.' && name[1] == '.'))
-      return directory_iterator_increment(it);
-    it.CurrentEntry.replace_filename(name);
+  } else if (CurDir != nullptr) {
+    StringRef Name(CurDir->d_name);
+    if ((Name.size() == 1 && Name[0] == '.') ||
+        (Name.size() == 2 && Name[0] == '.' && Name[1] == '.'))
+      return directory_iterator_increment(It);
+    It.CurrentEntry.replace_filename(Name, direntType(CurDir));
   } else
-    return directory_iterator_destruct(it);
+    return directory_iterator_destruct(It);
 
   return std::error_code();
 }
 
 ErrorOr<basic_file_status> directory_entry::status() const {
   file_status s;
   if (auto EC = fs::status(Path, s, FollowSymlinks))
     return EC;

llvm/trunk/lib/Support/Windows/Path.inc

return basic_file_status(file_type_from_attrs(FindData->dwFileAttributes),
                            perms_from_attrs(FindData->dwFileAttributes),
                            FindData->ftLastAccessTime.dwHighDateTime,
                            FindData->ftLastAccessTime.dwLowDateTime,
                            FindData->ftLastWriteTime.dwHighDateTime,
                            FindData->ftLastWriteTime.dwLowDateTime,
                            FindData->nFileSizeHigh, FindData->nFileSizeLow);
 }
 
-std::error_code detail::directory_iterator_construct(detail::DirIterState &it,
-                                                     StringRef path,
-                                                     bool follow_symlinks) {
-  SmallVector<wchar_t, 128> path_utf16;
+std::error_code detail::directory_iterator_construct(detail::DirIterState &IT,
+                                                     StringRef Path,
+                                                     bool FollowSymlinks) {
+  SmallVector<wchar_t, 128> PathUTF16;
 
-  if (std::error_code ec = widenPath(path, path_utf16))
-    return ec;
+  if (std::error_code EC = widenPath(Path, PathUTF16))
+    return EC;
 
   // Convert path to the format that Windows is happy with.
-  if (path_utf16.size() > 0 &&
-      !is_separator(path_utf16[path.size() - 1]) &&
-      path_utf16[path.size() - 1] != L':') {
-    path_utf16.push_back(L'\\');
-    path_utf16.push_back(L'*');
+  if (PathUTF16.size() > 0 &&
+      !is_separator(PathUTF16[Path.size() - 1]) &&
+      PathUTF16[Path.size() - 1] != L':') {
+    PathUTF16.push_back(L'\\');
+    PathUTF16.push_back(L'*');
   } else {
-    path_utf16.push_back(L'*');
+    PathUTF16.push_back(L'*');
   }
 
   //  Get the first directory entry.
   WIN32_FIND_DATAW FirstFind;
   ScopedFindHandle FindHandle(::FindFirstFileExW(
-      c_str(path_utf16), FindExInfoBasic, &FirstFind, FindExSearchNameMatch,
+      c_str(PathUTF16), FindExInfoBasic, &FirstFind, FindExSearchNameMatch,
       NULL, FIND_FIRST_EX_LARGE_FETCH));
   if (!FindHandle)
     return mapWindowsError(::GetLastError());
 
   size_t FilenameLen = ::wcslen(FirstFind.cFileName);
   while ((FilenameLen == 1 && FirstFind.cFileName[0] == L'.') ||
          (FilenameLen == 2 && FirstFind.cFileName[0] == L'.' &&
                               FirstFind.cFileName[1] == L'.'))
     if (!::FindNextFileW(FindHandle, &FirstFind)) {
       DWORD LastError = ::GetLastError();
       // Check for end.
       if (LastError == ERROR_NO_MORE_FILES)
-        return detail::directory_iterator_destruct(it);
+        return detail::directory_iterator_destruct(IT);
       return mapWindowsError(LastError);
     } else
       FilenameLen = ::wcslen(FirstFind.cFileName);
 
   // Construct the current directory entry.
-  SmallString<128> directory_entry_name_utf8;
-  if (std::error_code ec =
+  SmallString<128> DirectoryEntryNameUTF8;
+  if (std::error_code EC =
           UTF16ToUTF8(FirstFind.cFileName, ::wcslen(FirstFind.cFileName),
-                      directory_entry_name_utf8))
-    return ec;
+                      DirectoryEntryNameUTF8))
+    return EC;
 
-  it.IterationHandle = intptr_t(FindHandle.take());
-  SmallString<128> directory_entry_path(path);
-  path::append(directory_entry_path, directory_entry_name_utf8);
-  it.CurrentEntry = directory_entry(directory_entry_path, follow_symlinks,
-                                    status_from_find_data(&FirstFind));
+  IT.IterationHandle = intptr_t(FindHandle.take());
+  SmallString<128> DirectoryEntryPath(Path);
+  path::append(DirectoryEntryPath, DirectoryEntryNameUTF8);
+  IT.CurrentEntry =
+      directory_entry(DirectoryEntryPath, FollowSymlinks,
+                      file_type_from_attrs(FirstFind.dwFileAttributes),
 
   return std::error_code();
 }
 
-std::error_code detail::directory_iterator_destruct(detail::DirIterState &it) {
-  if (it.IterationHandle != 0)
+std::error_code detail::directory_iterator_destruct(detail::DirIterState &IT) {
+  if (IT.IterationHandle != 0)
     // Closes the handle if it's valid.
-    ScopedFindHandle close(HANDLE(it.IterationHandle));
-  it.IterationHandle = 0;
-  it.CurrentEntry = directory_entry();
+    ScopedFindHandle close(HANDLE(IT.IterationHandle));
+  IT.IterationHandle = 0;
+  IT.CurrentEntry = directory_entry();
   return std::error_code();
 }
 
-std::error_code detail::directory_iterator_increment(detail::DirIterState &it) {
+std::error_code detail::directory_iterator_increment(detail::DirIterState &IT) {
   WIN32_FIND_DATAW FindData;
-  if (!::FindNextFileW(HANDLE(it.IterationHandle), &FindData)) {
+  if (!::FindNextFileW(HANDLE(IT.IterationHandle), &FindData)) {
     DWORD LastError = ::GetLastError();
     // Check for end.
     if (LastError == ERROR_NO_MORE_FILES)
-      return detail::directory_iterator_destruct(it);
+      return detail::directory_iterator_destruct(IT);
     return mapWindowsError(LastError);
   }
 
   size_t FilenameLen = ::wcslen(FindData.cFileName);
   if ((FilenameLen == 1 && FindData.cFileName[0] == L'.') ||
       (FilenameLen == 2 && FindData.cFileName[0] == L'.' &&
                            FindData.cFileName[1] == L'.'))
-    return directory_iterator_increment(it);
+    return directory_iterator_increment(IT);
 
-  SmallString<128> directory_entry_path_utf8;
-  if (std::error_code ec =
+  SmallString<128> DirectoryEntryPathUTF8;
+  if (std::error_code EC =
           UTF16ToUTF8(FindData.cFileName, ::wcslen(FindData.cFileName),
-                      directory_entry_path_utf8))
-    return ec;
+                      DirectoryEntryPathUTF8))
+    return EC;
 
-  it.CurrentEntry.replace_filename(Twine(directory_entry_path_utf8),
-                                   status_from_find_data(&FindData));
+  IT.CurrentEntry.replace_filename(
+      Twine(DirectoryEntryPathUTF8),
+      file_type_from_attrs(FindData.dwFileAttributes),
   return std::error_code();
 }
 
 ErrorOr<basic_file_status> directory_entry::status() const {
   return Status;
 }
 
 static std::error_code nativeFileToFd(Expected<HANDLE> H, int &ResultFD,