[PATCH] D53363: [clangd] Encode Line/Column as a 32-bits integer.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(I think your math is off in the description: 20 bits should be 1M lines, not 4M)
I think this is a win, as I think truncation will be rare and not terrible. We should document the intentions around truncation though.

Incidentally, this means replacing just the StringRef in SymbolLocation::Position with a char* would save another 13%, because that's also 8 bytes.
(Obviously we'd probably replace the other StringRefs too, but it gives a lower bound).



================
Comment at: clangd/index/Index.h:37
+  //
+  // Position is encoded as a 32-bits integer like below:
+  // |<--- 20 bits --->|<-12 bits->|
----------------
I think it's enough to say "position is packed into 32 bits to save space".

It'd be useful to spell out the consequences in a second line.


================
Comment at: clangd/index/Index.h:41
   struct Position {
-    uint32_t Line = 0; // 0-based
+    static constexpr int LineBits = 20;
+    static constexpr int ColumnBits = 12;
----------------
(not sure these constants are needed as it stands - YAML shouldn't use them, see below)


================
Comment at: clangd/index/Index.h:46
     // Using UTF-16 code units.
-    uint32_t Column = 0; // 0-based
+    uint32_t Column : ColumnBits; // 0-based
   };
----------------
If we overflow the columns, it would be much easier to recognize if the result is always e.g. 255, rather than an essentially random number from 0-255 (from modulo 256 arithmetic).

This would mean replacing the raw fields with setters/getters, which is obviously a more invasive change. What about a compromise: add the setters, and call them from the most important codepaths: `SymbolCollector` and `Serialization`.


================
Comment at: clangd/index/YAMLSerialization.cpp:97
 
-template <> struct MappingTraits<SymbolLocation::Position> {
-  static void mapping(IO &IO, SymbolLocation::Position &Value) {
-    IO.mapRequired("Line", Value.Line);
-    IO.mapRequired("Column", Value.Column);
+struct NormalizedPosition {
+  using Position = clang::clangd::SymbolLocation::Position;
----------------
I don't think there's any reason to pack the YAML encoding.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53363