Page MenuHomePhabricator

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

Authored by sammccall on Sep 11 2018, 2:47 AM.

Details

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).

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Sep 11 2018, 2:47 AM
bkramer accepted this revision.Sep 12 2018, 10:51 AM

lg

lib/Support/Unix/Path.inc
708 ↗(On Diff #164828)

Funky formatting

This revision is now accepted and ready to land.Sep 12 2018, 10:51 AM
kristina requested changes to this revision.EditedSep 12 2018, 11:01 AM

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)

This revision now requires changes to proceed.Sep 12 2018, 11:01 AM
kristina added a comment.EditedSep 12 2018, 11:13 AM

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 marked 5 inline comments as done.

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

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 accepted this revision.Sep 12 2018, 12:34 PM

Thank you!

This revision is now accepted and ready to land.Sep 12 2018, 12:34 PM

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

kristina requested changes to this revision.EditedSep 12 2018, 1:46 PM

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!)

This revision now requires changes to proceed.Sep 12 2018, 1:46 PM
kristina added a comment.EditedSep 12 2018, 1:57 PM

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 updated this revision to Diff 165151.Sep 12 2018, 2:21 PM

Fix silly windows mistakes.

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)

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

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 added a comment.EditedSep 12 2018, 2:51 PM

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

This revision was not accepted when it landed; it landed in state Needs Review.Sep 12 2018, 3:09 PM
This revision was automatically updated to reflect the committed changes.
jordan_rose added inline comments.
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?

Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 3:48 PM
sammccall marked an inline comment as done.Jul 18 2019, 3:26 AM
sammccall added inline comments.
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.