hokein added inline comments.
Comment at: clangd/index/Index.h:52
+private:
+ friend class llvm::DenseMapInfo;
+
sammccall wrote:
> ioeric wrote:
> > warning: class 'DenseMapInfo' was previously declared as a struct
> > [-Wmismatched-tags]
> Fixed in rCTE320554
sammccall added inline comments.
Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+ // The symbol identifier, using USR.
malaperle wrote:
> sammccall wrote:
> > malaperle wrote:
> > > malaperle wrote:
> >
malaperle added inline comments.
Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+ // The symbol identifier, using USR.
sammccall wrote:
> malaperle wrote:
> > malaperle wrote:
> > > sammccall wrote:
> >
ioeric added inline comments.
Comment at: clangd/index/Index.h:52
+private:
+ friend class llvm::DenseMapInfo;
+
warning: class 'DenseMapInfo' was previously declared as a struct
[-Wmismatched-tags]
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.o
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE320486: [clangd] Introduce a "Symbol" class.
(authored by hokein, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D40897?vs=126538&id=126548#toc
Repository:
rCTE Clang Tools Extra
hokein marked an inline comment as done.
hokein added a comment.
I'm going to submit this patch to unblock the stuff in
https://reviews.llvm.org/D40548. Would be happy to address any further comments
afterwards.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40897
_
hokein marked an inline comment as done.
hokein added inline comments.
Comment at: clangd/index/CMakeLists.txt:5
+
+add_clang_library(clangdIndex
+ Index.cpp
sammccall wrote:
> hmm, I'm not sure whether we actually want this to be a separate library.
> This mea
sammccall added inline comments.
Comment at: clangd/index/CMakeLists.txt:5
+
+add_clang_library(clangdIndex
+ Index.cpp
hmm, I'm not sure whether we actually want this to be a separate library. This
means we can't depend on anything from elsewhere in clangd, an
hokein updated this revision to Diff 126538.
hokein added a comment.
- Get rid of clangdIndex library, using the existing clangDaemon library.
- Remove the getID() method.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40897
Files:
clangd/CMakeLists.txt
clangd/index/Index.c
hokein added inline comments.
Comment at: clangd/index/Index.cpp:34
+
+SymbolSlab::const_iterator SymbolSlab::find(const SymbolID& SymID) const {
+ return Symbols.find(SymID);
sammccall wrote:
> assert frozen? (and in begin())
We may not do the assert "frozen" i
hokein updated this revision to Diff 126519.
hokein marked 4 inline comments as done.
hokein added a comment.
Address remaining comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40897
Files:
clangd/CMakeLists.txt
clangd/index/CMakeLists.txt
clangd/index/Index.cpp
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
\o/
Comment at: clangd/index/Index.cpp:34
+
+SymbolSlab::const_iterator SymbolSlab::find(const SymbolID& SymID) const {
+ return Symbols.find(SymID);
hokein updated this revision to Diff 126378.
hokein marked 5 inline comments as done.
hokein added a comment.
Address comments on SymbolID.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40897
Files:
clangd/CMakeLists.txt
clangd/index/CMakeLists.txt
clangd/index/Index.cpp
sammccall added a comment.
Thanks for the restructuring? I want to take another pass today, but wanted to
mention some SymbolID things.
Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+ // The symbol identifier, using
malaperle added inline comments.
Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+ // The symbol identifier, using USR.
malaperle wrote:
> sammccall wrote:
> > hokein wrote:
> > > malaperle wrote:
> > >
hokein added a comment.
Reorganizing the source files made all the comments invalid in the latest
version :(. Feel free to comment on the old version or the new version.
Comment at: clangd/Symbol.h:23
+ // The path of the source file where a symbol occurs.
+ std::string File
hokein updated this revision to Diff 126156.
hokein marked 6 inline comments as done.
hokein added a comment.
- reorganize files, move to index subdirectory.
- change symbol ID to a hash value, instead of couple with USR.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40897
Fil
malaperle added a comment.
(From https://reviews.llvm.org/D40548) Here's the interface for querying the
index that I am using right now. It's meant to be able to retrieve from any
kind of "backend", i.e. in-memory, ClangdIndexDataStore, libIndexStore, etc. I
was able to implement "Open Workspac
sammccall added a comment.
More comments, but only two major things really:
- I'd like to clearly separate USR from SymbolID (even if you want to keep
using USRs as their basis for now)
- the file organization (code within files, and names of files) needs some work
I think
Everything else is d
hokein added a comment.
Thanks for the useful comments!
Comment at: clangd/Symbol.h:1
+//===--- Symbol.h ---*- C++-*-===//
+//
sammccall wrote:
> sammccall wrote:
> > I think that:
> > - there's other places in clang
hokein updated this revision to Diff 126115.
hokein marked 4 inline comments as done.
hokein added a comment.
Address review comments.
- Use SymbolSlab, for allowing future space optimization.
- Fix a Cast issue when building debug unittest.
Repository:
rCTE Clang Tools Extra
https://reviews
malaperle added inline comments.
Comment at: clangd/Symbol.h:23
+ // The path of the source file where a symbol occurs.
+ std::string FilePath;
+ // The offset to the first character of the symbol from the beginning of the
malaperle wrote:
> sammccall wrote:
>
malaperle added inline comments.
Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+ // The symbol identifier, using USR.
sammccall wrote:
> hokein wrote:
> > malaperle wrote:
> > > I think it would be nic
malaperle added a comment.
In https://reviews.llvm.org/D40897#946911, @hokein wrote:
> Our rough plan would be
>
> 1. Define the Symbol structure.
> 2. Design the interfaces of SymbolIndex, ASTIndex.
> 3. Combine 1) and 2) together to make global code completion work (we'd use
> YAML dataset for
sammccall added a comment.
Thanks for putting this together! Have a bit of a braindump here, happy to
discuss further either here or offline.
Comment at: clangd/Symbol.h:1
+//===--- Symbol.h ---*- C++-*-===//
+//
I t
ioeric added inline comments.
Comment at: clangd/Symbol.h:23
+ // The path of the source file where a symbol occurs.
+ std::string FilePath;
+ // The offset to the first character of the symbol from the beginning of the
Is this relative or absolute?
hokein added a comment.
In https://reviews.llvm.org/D40897#946708, @malaperle wrote:
> Hi! Have you looked into https://reviews.llvm.org/D40548 ? Maybe we need to
> coordinate the two a bit.
Hi Marc! Thanks for the input!
Yeah, Eric and I are working closely on a prototype of global code comp
malaperle added a comment.
Hi! Have you looked into https://reviews.llvm.org/D40548 ? Maybe we need to
coordinate the two a bit.
Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+ // The symbol identifier, using USR.
-
hokein created this revision.
Herald added subscribers: mgorny, klimek.
- The "Symbol" class represents a C++ symbol in the codebase, containing all
the information of a C++ symbol needed by clangd. clangd will use it in
clangd's AST/dynamic index and global/static index (code completion and cod
29 matches
Mail list logo
