[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:478
+public:
+  InMemoryNode(const std::string& FileName, InMemoryNodeKind Kind)
+      : Kind(Kind), FileName(llvm::sys::path::filename(FileName.data())) {}
----------------
NIT: accept a parameter of type `llvm::StringRef` instead of `const std::string&`.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:495
+  InMemoryFile(Status Stat, std::unique_ptr<llvm::MemoryBuffer> Buffer)
+      : InMemoryNode(Stat.getName().str(), IME_File), Stat(std::move(Stat)),
+        Buffer(std::move(Buffer)) {}
----------------
NIT: `.str()` is redundant (in fact, a pessimization) if parameter of `InMemoryNode` is a `StringRef`.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:522
+  InMemoryHardLink(StringRef Path, const InMemoryFile &ResolvedFile)
+      : InMemoryNode(Path.str(), IME_HardLink), ResolvedFile(ResolvedFile) {}
+  const InMemoryFile &getResolvedFile() const { return ResolvedFile; }
----------------
NIT: as noted in the other comment, `.str()` can be removed if parameter type is changed.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:570
   InMemoryDirectory(Status Stat)
-      : InMemoryNode(std::move(Stat), IME_Directory) {}
+      : InMemoryNode(Stat.getName().str(), IME_Directory),
+        Stat(std::move(Stat)) {}
----------------
NIT: `.str()` can be removed


================
Comment at: lib/Basic/VirtualFileSystem.cpp:662
   const auto ResolvedPerms = Perms.getValueOr(sys::fs::all_all);
+  // Cannot create HardLink from a directory.
+  if (HardLinkTarget && Buffer)
----------------
The comment looks outdated


================
Comment at: lib/Basic/VirtualFileSystem.cpp:663
+  // Cannot create HardLink from a directory.
+  if (HardLinkTarget && Buffer)
+    return false;
----------------
Shouldn't we assert this instead? This is not an invalid input, this is actually breaking our program logic, right? I.e. one shouldn't call addFile with HardLinkTarget set and non-null buffer 


================
Comment at: lib/Basic/VirtualFileSystem.cpp:677
+        if (HardLinkTarget)
+          Child.reset(new detail::InMemoryHardLink(P.str(), *HardLinkTarget));
+        else {
----------------
NIT: `.str()` seems redundant


================
Comment at: lib/Basic/VirtualFileSystem.cpp:804
+  return this->addFile(FromPath, 0, nullptr, None, None, None, None,
+                       static_cast<const detail::InMemoryFile *>(*ToNode));
+}
----------------
NIT: use `llvm::cast<InMemoryFile>` instead of static_cast.
It's essentially equivalent with added asserts.


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:974
+TEST_F(InMemoryFileSystemTest, AddHardLinkToFile) {
+  Twine FromLink = "/path/to/FROM/link";
+  Twine Target = "/path/to/TO/file";
----------------
Please use `StringRef`s everywhere. `Twine` should only be generally only be used as temporary objects, see the specific guidelines in Twine's docs.
Whenever a reference to an existing string is needed, a `StringRef` is the best choice.
If you need to create new strings (e.g. concatenation, etc.), use `std::string`.


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:976
+  Twine Target = "/path/to/TO/file";
+  StringRef content = "content of target";
+  FS.addFile(Target, 0, MemoryBuffer::getMemBuffer(content));
----------------
s/content/Content


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:987
+  StringRef content = "content of target file";
+  Twine link0 = "/path/to/0/link";
+  Twine link1 = "/path/to/1/link";
----------------
s/link0/Link0
.... and other `lowerCamelCase` vars --> `UpperCamelCase`


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:1090
+  }
+  EXPECT_EQ(1, Counts[0]);  // a
+  EXPECT_EQ(1, Counts[1]);  // b
----------------
This seems equivalent to `EXPECT_THAT(Nodes, UnorderedElementsAre("/a", "/a/b", "/c", "/c/d")`. Maybe simplify?

Note that iterators might give different file separators on windows. You might want to normalize, e.g. with `llvm::sys::path::native(..., Style::posix)`.
And still watch out for windows buildbot failures, in case the root (/) is interpreted differently on Windows, possibly giving different results.


Repository:
  rC Clang

https://reviews.llvm.org/D51359