[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 marked 16 inline comments as done.
usaxena95 added a comment.

Applied suggested changes.



================
Comment at: lib/Basic/VirtualFileSystem.cpp:653
+  // Cannot create HardLink from a directory.
+  if (HardLinkTarget && (!isa<detail::InMemoryFile>(HardLinkTarget) || Buffer))
+    return false;
----------------
ilya-biryukov wrote:
> HardLinkTarget can only be `InMemoryFile`, so maybe change the type of the parameter to `const InMemoryFile*`?
> Clients will be responsible for making sure the target is actually a file, but that seems fine, they already have to resolve the path to the target file.
> 
Moved InMemoryFile from detail:<anonymous> namespace to detail namespace in order to be visible in VirtualFileSystem.h


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:705
+  Twine To = Target;
+  auto OpenedFrom = FS->openFileForRead(From);
+  if (OpenedFrom.getError())
----------------
ilya-biryukov wrote:
> This matcher is pretty complicated and tests multiple things.
> If it fails, it might be hard to track down what exactly went wrong.
> 
> Given the name, it seems reasonable for it to simply test that both paths point to the same unique-id.
> We don't have to test every other invariant (sizes are the same, buffers are the same) on all the links we create, one test assertion per invariant should be enough (sizes are the same, buffers are the same, etc)

Removed the check for Buffer and size from the matcher.  Added the test for size and buffer in the smaller AddHardLinkTest


Repository:
  rC Clang

https://reviews.llvm.org/D51359