[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

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