[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: include/clang/Basic/VirtualFileSystem.h:359
 
+  /// Add a HardLink from one file to another.
+  /// Makes the UniqueID of To file same as that of From file. If CopyBuffer is
----------------
ilya-biryukov wrote:
> Maybe use the common spelling: "hard link"? Otherwise it feels like there is a HardLink class somewhere.
There are still instances of HardLink. Maybe update them too?


================
Comment at: include/clang/Basic/VirtualFileSystem.h:340
+               Optional<llvm::sys::fs::perms> Perms,
+               detail::InMemoryNode *HardLink);
 
----------------
- Maybe name it differently, e.g. ResolvedHardLink or HardLinkTarget?
- Maybe document that how this function behaves for non-null vs null values (i.e. creates dirs/files vs links to files)?


================
Comment at: include/clang/Basic/VirtualFileSystem.h:359
+  public:
+  /// Add a HardLink to a File.
+  /// The To path must be an existing file or a hardlink. The From file must not
----------------
Maybe document that it does nothing for directories?
Also, any reason to not support directories at this point? The limitation seem artificial at this point.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:485
   StringRef getFileName() const {
-    return llvm::sys::path::filename(Stat.getName());
+    return llvm::sys::path::filename(FileName.data());
   }
----------------
Maybe store the result of `llvm::sys::path::filename()` instead of the full name in the first place?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:709
+      if (auto Link = dyn_cast<detail::InMemoryHardLink>(Node)) {
+        return Link->getResolvedFile()->getBuffer()->getBuffer() ==
+               Buffer->getBuffer();
----------------
Why do we need to pass a buffer when creating a hard link?
Maybe just assert it's null?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:725
+  return addFile(P, ModificationTime,
+                 llvm::MemoryBuffer::getMemBuffer(
+                     Buffer->getBuffer(), Buffer->getBufferIdentifier()),
----------------
Why not pass the buffer unchanged? 


================
Comment at: lib/Basic/VirtualFileSystem.cpp:811
+    if (auto Link = dyn_cast<detail::InMemoryHardLink>(*Node))
+      return Link->getResolvedFile()->getStatus(Path.str());
+  }
----------------
`lookupInMemoryNode` never returns link nodes, right? Maybe assert the link case is not possible?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:846
+        CurrentEntry = Dir->getStatus(Path);
+      if (auto File = dyn_cast<detail::InMemoryFile>(I->second.get()))
+        CurrentEntry = File->getStatus(Path);
----------------
NIT: add else?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:848
+        CurrentEntry = File->getStatus(Path);
+      if (auto Link = dyn_cast<detail::InMemoryHardLink>(I->second.get()))
+        CurrentEntry = Link->getResolvedFile()->getStatus(Path);
----------------
NIT: add else?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:529
+public:
+  InMemoryHardLink(Status Stat, InMemoryNode *ResolvedNode)
+      : InMemoryNode(std::move(Stat), IME_HARD_LINK),
----------------
usaxena95 wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Why do we need the Stat?
> > > We can get it from `ResolveNode` if we want to store a copy. Any reason why it would be different?
> > ResolveNode can't be `null`, right? Maybe use a reference instead?
> > Also, maybe make it const?
> Changed it to reference. Can't make it const as the there it is involved in methods that are non const (like getResolvedFile returns a pointer, getBuffer is not const)
Both methods could be made const, though, right?
It looks like the buffers are always copied when returned from the file system (because the VFS interface returns a `unique_ptr<MemoryBuffer>`).
`getResolvedFile` can also return a const node.


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:701
+public:
+  void ExpectHardLink(Twine From, Twine To, const string &ExpectedBuffer) {
+    auto OpenedFrom = FS.openFileForRead(From);
----------------
Since gtest only reports one level of locations for the failing assertion, debugging test failures would be hard here (we won't see the actual test locations, the errors will always point into `ExpectHardLink` function.

We probably want to write a matcher and use `EXPECT_THAT`, so we can write something like
```
EXPECT_THAT(File, IsHardLinkTo(LinkTarget))
```

To write a matcher, we could use the gmock macros:
```
MATCHER_P(IsHardLinkTo, To, "") {
  llvm::StringRef From = arg;
  llvm::StringRef To = arg;
  // return true when matches, false otherwise
}
```


Repository:
  rC Clang

https://reviews.llvm.org/D51359