ioeric added inline comments.
Herald added a subscriber: kadircet.
Comment at: clangd/SourceCode.cpp:209
+ llvm::SmallString<128> RealPath;
+ if (SourceMgr.getFileManager().getVirtualFileSystem()->getRealPath(
+ Path, RealPath)) {
With the recent perfo
simark added inline comments.
Comment at: clangd/SourceCode.h:65
/// Get the absolute file path of a given file entry.
TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
hokein wrote:
> nit: this comment is not needed.
Woops, merge mistake.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE339483: [clangd] Avoid duplicates in findDefinitions
response (authored by simark, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48687?vs=160175&id=160197#toc
Repository:
rCTE
simark updated this revision to Diff 160175.
simark marked 4 inline comments as done.
simark added a comment.
Latest update.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48687
Files:
clangd/FindSymbols.cpp
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
u
simark marked an inline comment as done.
simark added a comment.
In https://reviews.llvm.org/D48687#1195840, @hokein wrote:
> Looks good with a few nits. Looks like you didn't update the patch correctly.
> You have marked comments done, but your code doesn't get changed accordingly.
> Please do
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
Looks good with a few nits. Looks like you didn't update the patch correctly.
You have marked comments done, but your code doesn't get changed accordingly.
Please double check with it.
I trie
simark marked 8 inline comments as done.
simark added inline comments.
Comment at: clangd/SourceCode.h:69
+llvm::Optional getRealPath(const FileEntry *F,
+const SourceManager &SourceMgr);
ilya-biryukov wrote:
> This funct
ilya-biryukov added a comment.
Only a few NITs from my side.
Excited for this fix to get in, have been seeing duplicate in other cases too
:-)
Comment at: clangd/SourceCode.h:69
+llvm::Optional getRealPath(const FileEntry *F,
+const So
simark updated this revision to Diff 159972.
simark marked an inline comment as done.
simark added a comment.
Replace
RelPathPrefix == StringRef()
with
RelPathPrefix.empty()
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48687
Files:
clangd/FindSymbols.cpp
clangd/Sou
simark marked 3 inline comments as done.
simark added a comment.
In https://reviews.llvm.org/D48687#1193470, @hokein wrote:
> Sorry for the delay. I thought there was a dependent patch of this patch, but
> it seems resolved, right?
>
> A rough round of review.
Right, the patch this one depends
hokein added a comment.
Sorry for the delay. I thought there was a dependent patch of this patch, but
it seems resolved, right?
A rough round of review.
> New version. I tried to share the code between this and SymbolCollector, but
> didn't find a good clean way. Do you have concrete suggestio
simark updated this revision to Diff 159768.
simark added a comment.
Formatting.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48687
Files:
clangd/FindSymbols.cpp
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/XRefs.cpp
unittests/clangd/TestFS.cpp
unittests/clang
simark updated this revision to Diff 159764.
simark added a comment.
Herald added a subscriber: arphaman.
New version. I tried to share the code between this and SymbolCollector, but
didn't find a good clean way. Do you have concrete suggestions on how to do
this? Otherwise, would it be accepta
simark updated this revision to Diff 155057.
simark added a comment.
This is an update of my work in progress. I haven't done any sharing with
SymbolCollector, as it's not clear to me how to proceed with that.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48687
Files:
clang
simark added a comment.
Herald added a subscriber: omtcyfz.
I took a look at `SymbolCollector`'s `toURI`, and I am not sure what to get
from it. It seems like a lot of it could be replaced with a call to
`FileSystem::getRealPath`.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/
ilya-biryukov added a comment.
> I think the fixing way is to normalize the file path from AST (making it
> absolute).
Totally agree. Could we run the code used to get the URI to store in the
dynamic index? Should we expose and reuse code in `getSymbolLocation()` from
`SymbolCollector.cpp`?
simark added a comment.
An update, I traced the difference in behavior to the difference in how
`RealFileSystem` and `InMemoryFileSystem` return `Status`es.
I uploaded a patch to change `InMemoryFileSystem` to work like
`RealFileSystem`: https://reviews.llvm.org/D48903
With this patch applied
simark added a comment.
In https://reviews.llvm.org/D48687#1150960, @hokein wrote:
> After taking a look closely, I figured why there are two candidates here --
> one is from AST (the one with ".." path); the other one is from dynamic
> index, the deduplication failed because of the different p
hokein added a comment.
After taking a look closely, I figured why there are two candidates here -- one
is from AST (the one with ".." path); the other one is from dynamic index, the
deduplication failed because of the different paths :(
I think the fixing way is to normalize the file path from
simark added a comment.
FYI, I have uploaded my attempt at writing a unit test here:
https://reviews.llvm.org/D48726
I tried to make it as close as possible to the example in the bug report, but
don't manage to reproduce the bug.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/
simark added a comment.
Btw, this seems to happen only when the included header is in the preamble. If
I put a variable declaration before `#include "first.h"`, things work as
expected, we have not duplicates and the path is normalized.
Repository:
rCTE Clang Tools Extra
https://reviews.ll
ilya-biryukov added a comment.
I'm somewhat familiar with the internals of clang around the FileManager/VFS,
so I could try creating the repro of this issue. The bugreport has enough info
to get started. (Probably tomorrow, I certainly won't get to it today).
In https://reviews.llvm.org/D48687#
hokein added a comment.
> Could we try to figure out why the duplicates were there in the first place
> and why the paths were different?
+1, I think there are two issues here:
1. the result contains two candidates, which should be one, IIUC.
2. the absolute file path problem, we encountered si
malaperle added a comment.
In https://reviews.llvm.org/D48687#1146515, @simark wrote:
> 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 diffe
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
ilya-biryukov added a comment.
Also some heads-up: this would probably conflict
https://reviews.llvm.org/D47846 that moves the same function into a different
file.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48687
___
cfe-commi
ilya-biryukov added a comment.
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?
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 tha
simark created this revision.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov.
When compile_commands.json contains some source files expressed as
relative paths, we can get duplicate responses to findDefinitions. The
responses only differ by the URI, which are diffe
28 matches
Mail list logo
