simark added a comment.
In https://reviews.llvm.org/D48687#1146308, @ilya-biryukov wrote:
> Thanks for the patch!
> Could we try to figure out why the duplicates were there in the first place
> and why the paths were different?
I tried to do that, but it goes deep in the clang internals with which I'm not
familiar. All I could see was that when creating the `FileEntry` representing
the `/home/emaisin/src/ls-interact/cpp-test/build/../src/first.h` file,
`FileManager::getFile` is called with `OpenFile=false`. This makes it so that
the `RealPathName` field is not set (at `FileManager.cpp:320`). Because
`RealPathName` is not set (well, empty), we use the non-normalized name in
`getAbsoluteFilePath`. That's all I can tell.
> It should be easy to mock exactly the same setup you have in #37963, i.e.
> create a vfs with three files and compilation database that has exactly those
> compile commands. You mention you tried to repro the issue, did you try doing
> that with the unit-test or a lit-test?
I tried doing a unit test that mimics the setup in #37963. For some reason I
can't explain, the header file would always come out "correct". If you want to
investigate further, I can update the patch with what I have so far.
> After looking at the makefile, my guess would be that the problem comes from
> the paths starting with `../` inside the compilation database.
Yes. But AFAIK it's valid (it's relative to `directory`, which is the build
directory. The compile_commands.json is generated with Bear.
================
Comment at: clangd/XRefs.cpp:179
+ const SourceManager &SourceMgr,
+ const vfs::FileSystem &VFS) {
SmallString<64> FilePath = F->tryGetRealPathName();
----------------
ilya-biryukov wrote:
> Do we really need an extra vfs param?
> Could we use `SourceMgr.getFileManager().getVirtualFileSystem()` instead?
>
>
Ah probably not. I couldn't find a way to get a handle on the VFS, but that
looks good.
================
Comment at: unittests/clangd/TestTU.h:47
- ParsedAST build() const;
+ ParsedAST build(IntrusiveRefCntPtr<vfs::FileSystem> *OutFS = nullptr) const;
SymbolSlab headerSymbols() const;
----------------
ilya-biryukov wrote:
> We don't need an extra output param here.
> There's a way to get the vfs from the ASTContext:
> `ParsedAST().getASTContext().getSourceManager().getFileManager().getVirtualFileSystem()`.
Thanks. It's just not obvious :).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48687
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
