OikawaKirie added a comment.
Replies from the original author Hao Zhang <zhangha...@ios.ac.cn>
--------------------------------------------------------------
> ...split the data structures between relative and absolute paths. The
> existing data structures would only store absolute paths, but there are new
> ones for relative paths that can be cached and swapped out.
I like the idea that using one cache for absolute paths and multiple caches for
relative paths. Better than my approach.
> Another point is that FileManager takes the working directory at construction
> time via FileSystemOptions. There's a FIXME to remove that argument, but (as
> you've seen) the assumption is baked in that the CWD doesn't change. I think
> the current patch may not completely fix it, since you can't modify
> FileSystemOptions.
It's the point. It is the assumption that the CWD won't change leads to the
problem. I agree with you that `FileSystemOptions` should be removed and grab
the CWD directly from the VFS.
> Another option here is to assume the VFS never changes directories (no need
> to store the CWD) ... Then if ClangTool wants to change directories, it would
> be expected to maintain an appropriate set of VFS layers.
IIUC, does this mean that clients should create new `Filesystem`s when they
change directories? It looks to me that it might conflict with the design of
`Filesystem` since it already provides an API to change the CWD.
Another option (which I mentioned in the previous post) is to use absolute
paths to query the cache, something like this:
llvm::ErrorOr<const FileEntry *>
FileManager::getFile(StringRef Filename, bool openFile, bool CacheFailure) {
// Froce using the absolute path to query the cache
llvm::SmallString<128> AbsPath(Filename);
makeAbsolutePath(AbsPath);
auto Result = getFileRef(AbsPath.str(), openFile, CacheFailure);
if (Result)
return &Result->getFileEntry();
return llvm::errorToErrorCode(Result.takeError());
}
It is more likely a defensive workaround to this problem. It might not be a
good solution to the real problem (the assumption that the CWD won't change).
The above code resulted in failures of some test cases, I haven't looked
closely into why they failed. In my point of view, `Filename` should only be
used as the key to query the cache. Logically, if I change it into the absolute
path, those test cases should pass as usual.
Anyway, your approach (remove `FileSystemOptions`, one cache for absolute
paths, multiple caches for relative paths) looks good to me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92160/new/
https://reviews.llvm.org/D92160
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
