JDevlieghere added a comment.
Overall this looks good. I wonder if abstracting the ExternalRedirect as a
small wrapper class around a SmallString would help. There's a few operations
that are repeated, like the example below, and it could also house the logic
that's currently in the lambda.
if (ExternalRedirect.empty()) {
assert(RE->getKind() == RedirectingFileSystem::EK_File);
ExternalRedirect = RE->getExternalContentsPath();
}
One thing I found tricky during the review was differentiating between places
where the ExternalRedirect was computed and uses. Looking at the signatures the
`lookupPath` functions are computing it and `status` is consuming it, although
the latter then changes the StringRef (which I guess shouldn't persist?). I
don't know if the wrapper could alleviate this as well.
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:564
///
+/// Re-mapped directories are represented as
+/// /// \verbatim
----------------
I think we should expand on this a bit more to contrast a directory-remapped
directory vs a file-remapped directory (for a lack of better term).
Specifically, an empty file-remapped directory could be confusing, as it
behaves quite differently but looks pretty similar in YAML.
```
{
'type': 'file',
'name': <string>,
'external-contents': <path to external file>
}
```
```
{
'type': 'directory',
'name': <string>,
'contents': []
}
```
Looking at the two, I wonder if there would be any benefit to changing the
`type` in the yaml to match the `NameKind`.
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:624
+
+ /// whether to use the external path as the name for this file or
directory.
+ bool useExternalName(bool GlobalUseExternalName) const {
----------------
nit: s/whether/Whether/
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:776-778
/// Looks up \p Path in \c Roots.
- ErrorOr<Entry *> lookupPath(const Twine &Path) const;
+ ErrorOr<Entry *> lookupPath(const Twine &Path,
+ SmallVectorImpl<char> &ExternalRedirect) const;
----------------
dexonsmith wrote:
> It's not entirely clear what the extra parameters do / when they're filled in
> / etc; can you add a bit more documentation?
>
> Also, I see that the public caller ignores the `ExternalRedirect` parameter.
> I wonder if this should be renamed to `lookupPathWithName` or
> `lookupPathImpl`, leaving behind a `lookupPath` wrapper that creates the
> ignored buffer for the public interface... no strong opinion though.
+1.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94844/new/
https://reviews.llvm.org/D94844
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
