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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
