[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Could we try removing `Status` from the base class and move it into `InMemoryFile` and `InMemoryDir`?
It shouldn't be too much work and would give safer interfaces for our new hierarchy.



================
Comment at: lib/Basic/VirtualFileSystem.cpp:470
 
-enum InMemoryNodeKind { IME_File, IME_Directory };
+enum InMemoryNodeKind { IME_File, IME_Directory, IME_HARD_LINK };
 
----------------
NIT: maybe use a name that follow the naming style for the other node kinds, i.e. IME_HardLink?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:526
+  InMemoryNode *ResolvedNode;
+  StringRef Name;
+
----------------
Can we get away without storing `Name`? `toString()` can just say `"hard link to "  + ResolveNode->toString()`


================
Comment at: lib/Basic/VirtualFileSystem.cpp:529
+public:
+  InMemoryHardLink(Status Stat, InMemoryNode *ResolvedNode)
+      : InMemoryNode(std::move(Stat), IME_HARD_LINK),
----------------
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?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:529
+public:
+  InMemoryHardLink(Status Stat, InMemoryNode *ResolvedNode)
+      : InMemoryNode(std::move(Stat), IME_HARD_LINK),
----------------
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?


================
Comment at: lib/Basic/VirtualFileSystem.cpp:535
+  InMemoryNode *getResolvedNode() { return ResolvedNode; }
+  llvm::MemoryBuffer *getBuffer() {
+    return static_cast<InMemoryFile *>(ResolvedNode)->getBuffer();
----------------
Could we inline `getBuffer` and `getStatus`? Having the client code that's explicit about resolving the references seems like a good idea.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:639
+                                 Optional<llvm::sys::fs::perms> Perms,
+                                 Optional<detail::InMemoryNode *> HardLink) {
   SmallString<128> Path;
----------------
Why not just `InMemoryNode*`? Null would be the empty hardlink


================
Comment at: lib/Basic/VirtualFileSystem.cpp:782
+    // If Node is HardLink then return the resolved link.
+    if (auto File = dyn_cast<detail::InMemoryHardLink>(Node)) {
+      if (I == E)
----------------
NIT: maybe call the variable Link or HardLink?
Since it's not a file.


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:1054
+  EXPECT_FALSE(FS.addHardLink(link, dir));
+}
+
----------------
Maybe also add a test that gets into hard links via directory iterators?
To make sure those give the same results as `openFileForRead`


Repository:
  rC Clang

https://reviews.llvm.org/D51359