bruno added a comment.
Hi Volodymyr,
Thanks for working on this, really nice work with the tests! Comments below.
> - No support for 'fallthrough' in crash reproducer.
That'd be nice to have at some point to make sure we never escape into the real
fs.
> - Current way of working with modules in VFS "root" is clunky and error-prone.
Why?
> Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator
> but doesn't share code. At the moment I think it's not worth it to rework
> those abstractions to make more code reusable. If you've noticed problems
> with those iterators before, maybe it makes sense to try to find a better
> approach.
Maybe add FIXMEs for it?
================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:934
RedirectingFileSystem &FS;
RedirectingDirectoryEntry::iterator Current, End;
+ bool IsExternalFSCurrent;
----------------
Can you add comments to these new members? Specifically, what's the purpose of
`IsExternalFSCurrent` and how it connects to `ExternalFS`
================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:940
- std::error_code incrementImpl();
+ std::error_code incrementExternal();
+ std::error_code incrementContent(bool IsFirstTime);
----------------
Same for these methods
================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:1738
+ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path,
+ bool ShouldCheckExternalFS) {
ErrorOr<Entry *> Result = lookupPath(Path);
----------------
Is passing `ShouldCheckExternalFS ` really necessary? Why checking the status
of the member isn't enough?
================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:2041
+ IsExternalFSCurrent(false), ExternalFS(ExternalFS) {
+ EC = incrementImpl(true);
}
----------------
`incrementImpl(true/*IsFirstTime*/)`
================
Comment at: clang/lib/Basic/VirtualFileSystem.cpp:2045
std::error_code VFSFromYamlDirIterImpl::increment() {
- assert(Current != End && "cannot iterate past end");
- ++Current;
- return incrementImpl();
+ return incrementImpl(false);
+}
----------------
`incrementImpl(false/*IsFirstTime*/)`
https://reviews.llvm.org/D50539
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
