[PATCH] D117650: [llvm][vfs] Implement in-memory symlinks

[Jan Svoboda via Phabricator via llvm-commits]

jansvoboda11 marked 4 inline comments as done.
jansvoboda11 added inline comments.
Herald added a project: All.


================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:493
+  /// directory for \param To isn't automatically created.
+  bool addSymbolicLink(const Twine &From, const Twine &To,
+                       time_t ModificationTime, Optional<uint32_t> User = None,
----------------
sammccall wrote:
> FWIW I find `From`/`to` confusing, because I had to rewire my brain to think of `ln` as a variant of `cp` in order to remember which order the args go in :-)
> 
> I think `NewLink`/`Target` would be much clearer, but there's value in being consistent with the existing `addHardLink` too, so up to you.
You're right, I struggle with that too. I changed that to `NewLink`/`Target` for both the new `addSymbolicLink()` and the existing `addHardLink()`. Let me know if you'd prefer to extract the change to `addHardLink()` into a prep-commit. (I assume review wouldn't be necessary.)


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:902
+                   detail::InMemoryDirectory *RootDir, const Twine &P,
+                   bool FollowSymlinks, size_t SymlinkDepth = 0) {
   SmallString<128> Path;
----------------
sammccall wrote:
> FollowFinalSymlink?
> It's wordy, but it's not clear to me what the behavior is if you look up "link/link/file" with FollowSymlinks=false. Also the plural is a bit misleading.
> 
> It might be worth adding a comment here with the most important contract: if Follow is true, then it will never return a symlink node.
Fair enough, changed.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:983
   // before. Resolved ToPath must be a File.
   if (!ToNode || FromNode || !isa<detail::InMemoryFile>(*ToNode))
     return false;
----------------
sammccall wrote:
> This fails if ToNode is a symbolic link. Linux allows hardlinks to symlinks, apparently macOS resolves the symlink and hardlinks to the target, posix allows either (implementation-defined) but doesn't allow it to just fail.
> 
> This isn't posix so you can do whatever you like, but I think the choice deserves a comment.
Good catch! I chose to follow symlinks to be consistent with macOS for now.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1056
       case detail::IME_HardLink:
+      case detail::IME_SymbolicLink:
         Type = sys::fs::file_type::regular_file;
----------------
sammccall wrote:
> This doesn't look correct to me, if the symlink points to a directory we'll claim it's a file and then operations on it will fail.
> 
> Since the VFS interface doesn't know about symlinks, it's probably best to resolve the symlink (chain) here and report the target. But returning symlink_file might be ok...
> 
> If you resolve and it dangles, you have to choose between reporting regular_file, symlink_file, or hiding the entry entirely... not really sure which is best.
I chose to resolve the symlink to keep the VFS interface symlink-free. In case of dangling symlink, I think it would make sense to report `type_unknown`, WDYT?

BTW, since I chose to resolve the symlink and `directory_entry` needs the target path, I was forced to tweak the `lookupInMemoryNode` to return the resolved target path as well, since with symlinks it will differ from the requested path.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117650/new/

https://reviews.llvm.org/D117650