[PATCH] D45513: [clangd] Add line and column number to the index symbol.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/Index.h:27
 struct SymbolLocation {
+  struct Position {
+    // Line position in a document (zero-based).
----------------
There are 4 of these per symbol, if we can keep line + character to 32 bits we save 16 bytes per symbol.

That looks like it might still be ~10% of non-string size. WDYT?
(12 bits for column and 20 bits for line seems like a reasonable guess)


================
Comment at: clangd/index/Index.h:28
+  struct Position {
+    // Line position in a document (zero-based).
+    int Line = 0;
----------------
These comments seem pretty obvious, I think
`int Line=0, Character=0 // zero-based` is enough, but up to you.

I would like a comment explaining why we're storing these redundant representations though.
e.g. `// Storing Line/Character allows us to build LSP responses without reading the file content.`




================
Comment at: clangd/index/Index.h:32
+    // Character offset on a line in a document (zero-based).
+    int Character = 0;
+  };
----------------
Column?

LSP calls this "character" but this is nonstandard and I find it very confusing with offset. 


================
Comment at: clangd/index/Index.h:39
   // using half-open range, [StartOffset, EndOffset).
+  // FIXME(hokein): remove these fields in favor of Position.
   unsigned StartOffset = 0;
----------------
I don't think we should remove them, we'll just have the same problem in reverse.
Could position have have line/col/offset, and have Position Start, End?


================
Comment at: clangd/index/SymbolCollector.cpp:206
+  Result.EndPos.Line = Result.StartPos.Line;
+  Result.EndPos.Character = Result.StartPos.Character + TokenLength;
+
----------------
I don't think this works, tokens can be split across lines.

I believe you want to compute NameLoc.locWithOffset(TokenLength) and then decompose that into line/col.
(getLocForEndOfToken, confusingly, is different)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45513