ilya-biryukov added a comment.
Thanks, the interface and implementation look good!
Last drop of nits and suggestions for code simplification.
================
Comment at: include/clang/Basic/VirtualFileSystem.h:359
+ public:
+ /// Add a HardLink to a File.
+ /// The To path must be an existing file or a hardlink. The From file must
not
----------------
usaxena95 wrote:
> ilya-biryukov wrote:
> > Maybe document that it does nothing for directories?
> > Also, any reason to not support directories at this point? The limitation
> > seem artificial at this point.
> Links to directories cannot be added with the current logic. Resolving paths
> and iterating directories becomes non trivial. Current implementation
> supports the use case of "file with multiple names" which is mostly
> sufficient to mimic the compilation.
You're right. E.g. one potential trouble is cycles in the directory tree.
Let's leave them out, directory links do seem like some hard work and we don't
have a use-case for them.
================
Comment at: include/clang/Basic/VirtualFileSystem.h:364
+ /// The To path must be an existing file or a hardlink. The From file must
not
+ /// have been added before. The From Node is added as an InMemoryHardLink
+ /// which points to the resolved file of To Node. The From path or the To
Path
----------------
InMemoryHardLink is a private implementation detail. Maybe remove it from the
docs of the public API?
================
Comment at: include/clang/Basic/VirtualFileSystem.h:369
+ /// successfully created, false otherwise.
+ bool addHardLink(const Twine &From, const Twine &To,
+ Optional<uint32_t> User = None,
----------------
Maybe expand a little on what a "hard link" is in our implementation?
Specifically, the important bits seem to be:
They are not intended to be fully equivalent to hard links of the classical
filesystems, despite the name.
Both the original file and a hard link have the same UniqueID returned in their
stats, share the same buffer, etc.
There is no way to distinguish hard links and the original files after they
were added.
(this is already in the docs) Only links to files are supported, directories
cannot be linked.
================
Comment at: include/clang/Basic/VirtualFileSystem.h:370
+ bool addHardLink(const Twine &From, const Twine &To,
+ Optional<uint32_t> User = None,
+ Optional<uint32_t> Group = None,
----------------
Maybe remove these defaulted parameters? They're not used and we'll probably
have confusing semantics even if we add support for them (since our hard links
don't have their own status, and merely return the status of the original file).
WDYT?
================
Comment at: lib/Basic/VirtualFileSystem.cpp:476
InMemoryNodeKind Kind;
-
-protected:
- /// Return Stat. This should only be used for internal/debugging use. When
- /// clients wants the Status of this node, they should use
- /// \p getStatus(StringRef).
- const Status &getStatus() const { return Stat; }
+ string FileName;
----------------
Please use std::string, we usually don't leave out the std:: namespace in LLVM.
================
Comment at: lib/Basic/VirtualFileSystem.cpp:523
+ : InMemoryNode(Path.str(), IME_HardLink), ResolvedFile(ResolvedFile) {}
+ const InMemoryFile *getResolvedFile() const { return &ResolvedFile; }
+
----------------
Maybe return a reference here too? To indicate this can't be null.
================
Comment at: lib/Basic/VirtualFileSystem.cpp:653
+ // Cannot create HardLink from a directory.
+ if (HardLinkTarget && (!isa<detail::InMemoryFile>(HardLinkTarget) || Buffer))
+ return false;
----------------
HardLinkTarget can only be `InMemoryFile`, so maybe change the type of the
parameter to `const InMemoryFile*`?
Clients will be responsible for making sure the target is actually a file, but
that seems fine, they already have to resolve the path to the target file.
================
Comment at: lib/Basic/VirtualFileSystem.cpp:665
// End of the path, create a new file or directory.
Status Stat(P.str(), getNextVirtualUniqueID(),
llvm::sys::toTimePoint(ModificationTime), ResolvedUser,
----------------
We don't use `Status` for links, maybe move creation of `Status` into the
branch that handles files?
This would allow to assert `Buffer` is not null too
================
Comment at: lib/Basic/VirtualFileSystem.cpp:690
getNextVirtualUniqueID(), llvm::sys::toTimePoint(ModificationTime),
- ResolvedUser, ResolvedGroup, Buffer->getBufferSize(),
- sys::fs::file_type::directory_file, NewDirectoryPerms);
+ ResolvedUser, ResolvedGroup, 0, sys::fs::file_type::directory_file,
+ NewDirectoryPerms);
----------------
Oh, wow, this used to set the size of the created parent directories to the
size of the buffer we're creating. I guess nobody looks at it anyway, so we
were never hitting any problems because of this.
Good catch, thanks for fixing this.
================
Comment at: lib/Basic/VirtualFileSystem.cpp:803
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
- if (Node)
- return (*Node)->getStatus(Path.str());
+ if (Node) {
+ assert(!isa<detail::InMemoryHardLink>(*Node));
----------------
NIT: maybe invert condition?
I.e. change to
```
if (!Node)
return Node.getError();
//.. rest of the code
```
to keep the amount of code with higher nesting small. See somewhat related
section in [[
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
| LLVM Style Guide ]]
================
Comment at: lib/Basic/VirtualFileSystem.cpp:804
+ if (Node) {
+ assert(!isa<detail::InMemoryHardLink>(*Node));
+ if (auto Dir = dyn_cast<detail::InMemoryDirectory>(*Node))
----------------
This line seems to have wrong indent.
Running `git-clang-format` before submitting the code is a convenient way to
fix this.
================
Comment at: lib/Basic/VirtualFileSystem.cpp:809
+ return File->getStatus(Path.str());
+ }
return Node.getError();
----------------
NIT: add `llvm_unreachable()` to make it clear we don't have a 4th kind of node?
================
Comment at: lib/Basic/VirtualFileSystem.cpp:841
llvm::sys::path::append(Path, I->second->getFileName());
- CurrentEntry = I->second->getStatus(Path);
+ if (auto Dir = dyn_cast<detail::InMemoryDirectory>(I->second.get()))
+ CurrentEntry = Dir->getStatus(Path);
----------------
Maybe extract it into a small helper function?
It only pops up twice, but a helper should still give better readability for
both cases.
================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:702
+MATCHER_P3(IsHardLinktTo, FS, Target, ExpectedBuffer, "") {
+ Twine From = arg;
----------------
s/IsHardLinktTo/IsHardLinkTo
================
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:705
+ Twine To = Target;
+ auto OpenedFrom = FS->openFileForRead(From);
+ if (OpenedFrom.getError())
----------------
This matcher is pretty complicated and tests multiple things.
If it fails, it might be hard to track down what exactly went wrong.
Given the name, it seems reasonable for it to simply test that both paths point
to the same unique-id.
We don't have to test every other invariant (sizes are the same, buffers are
the same) on all the links we create, one test assertion per invariant should
be enough (sizes are the same, buffers are the same, etc)
Repository:
rC Clang
https://reviews.llvm.org/D51359
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
