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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits