[PATCH] D51359: Adding HardLink Support to VirtualFileSystem.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM with a last drop of NITs.
Do you already have commit access or do you want me to submit this change for you?



================
Comment at: include/clang/Basic/VirtualFileSystem.h:369
+  /// The To path must be an existing file or a hardlink. The From file must not
+  /// have been added before. The From path or the To Path must not be a
+  /// directory. The From Node is added as a hard link which points to the
----------------
NIT: since From should not exist, a comment that it shouldn't be a directory is redundant.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:677
+        if (HardLinkTarget)
+          Child.reset(new detail::InMemoryHardLink(P.str(), *HardLinkTarget));
+        else {
----------------
usaxena95 wrote:
> ilya-biryukov wrote:
> > NIT: `.str()` seems redundant
> Here P is a Twine here which cannot be converted to StringRef
You're right. Sorry for the confusion.


================
Comment at: lib/Basic/VirtualFileSystem.cpp:485
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const { return FileName.data(); }
+  InMemoryNodeKind getKind() const { return Kind; }
----------------
NIT: `.data()` is redundant (in fact, a pessimization, since StringRef ctor will have to compute the length of the string), string can be implicitly converted to StringRef


================
Comment at: lib/Basic/VirtualFileSystem.cpp:613
+    return Dir->getStatus(RequestedName);
+  else if (auto File = dyn_cast<detail::InMemoryFile>(Node))
+    return File->getStatus(RequestedName);
----------------
NIT: else can be removed, same at the next statement. See the [[ https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return | style guide ]]


================
Comment at: lib/Basic/VirtualFileSystem.cpp:663
+  // HardLink cannot have a buffer of its own.
+  assert(!(HardLinkTarget && Buffer));
   // Any intermediate directories we create should be accessible by
----------------
NIT: maybe put comment into an assertion message?
E.g. `assert(!(HardLinkTarget && Buffer) && "HardLink cannot have a buffer")`


================
Comment at: lib/Basic/VirtualFileSystem.cpp:673
       if (I == E) {
-        // End of the path, create a new file or directory.
-        Status Stat(P.str(), getNextVirtualUniqueID(),
-                    llvm::sys::toTimePoint(ModificationTime), ResolvedUser,
-                    ResolvedGroup, Buffer->getBufferSize(), ResolvedType,
-                    ResolvedPerms);
+        // End of the path
         std::unique_ptr<detail::InMemoryNode> Child;
----------------
NIT: end a comment with a full stop


================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:708
+  return !(OpenedFrom.getError() || OpenedTo.getError() ||
+           (*OpenedFrom)->status()->getUniqueID() !=
+               (*OpenedTo)->status()->getUniqueID());
----------------
This is effectively a double negation, which is a bit hard to read.
Maybe replace `!(a || b != c)` to `!a && b == c` to avoid it? 


Repository:
  rC Clang

https://reviews.llvm.org/D51359