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