[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for the change. Having something like this makes total sense.

Mutating existing in-memory nodes looks shaky and requires making `Status` mutable, which also looks undesirable.
Maybe we could consider adding a new kind of `InMemoryNode` for hard links that would store the link target? We would have to "resolve" the link on each access, but that would probably allow to avoid changing `Status` interfaces and get rid of the  `UniqueID -> set of files' map. WDYT?

Also some generic comments wrt to the coding style, naming, etc.



================
Comment at: include/clang/Basic/VirtualFileSystem.h:80
 
+  void setUniqueID(llvm::sys::fs::UniqueID UID) { this->UID = UID; }
+  void setSize(uint64_t Size) { this->Size = Size; }
----------------
It seems `Status` was designed to be immutable.
Can we copy or reassign the whole status at the points where we need it?


================
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
----------------
Maybe use the common spelling: "hard link"? Otherwise it feels like there is a HardLink class somewhere.


================
Comment at: include/clang/Basic/VirtualFileSystem.h:362
+  /// true then contents of from buffer is copied into the buffer of To file.
+  void addHardlink(const Twine &From, const Twine &To, bool CopyBuffer);
+
----------------
The links seem to only work for files at this point.
Maybe explicitly document what happens with directories?
Also, maybe document what happens with existing hard links and existing files?


================
Comment at: include/clang/Basic/VirtualFileSystem.h:362
+  /// true then contents of from buffer is copied into the buffer of To file.
+  void addHardlink(const Twine &From, const Twine &To, bool CopyBuffer);
+
----------------
ilya-biryukov wrote:
> The links seem to only work for files at this point.
> Maybe explicitly document what happens with directories?
> Also, maybe document what happens with existing hard links and existing files?
Maybe s/addHardlink/addHardLink?


================
Comment at: include/clang/Basic/VirtualFileSystem.h:468
 #endif // LLVM_CLANG_BASIC_VIRTUALFILESYSTEM_H
+
----------------
Accidental whitespace change?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:516
 
+  void setBuffer(std::unique_ptr<llvm::MemoryBuffer> buffer) {
+    this->Buffer = std::move(buffer);
----------------
s/buffer/Buffer


================
Comment at: lib/Basic/VirtualFileSystem.cpp:645
+  // Merging ToIDSet into FromIDSet.
+  for (detail::InMemoryNode *to_node : ToIDSet) {
+    to_node->setUniqueId(FromStat->getUniqueID());
----------------
s/to_node/ToNode


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:1608
 }
+
----------------
Accidental whitespace change?


Repository:
  rC Clang

https://reviews.llvm.org/D51359