kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:121
+ template <typename T>
+ LLVM_NODISCARD bool consumeSize(T &Container, unsigned MinSize = 1) {
+ auto Size = consumeVar();
----------------
regarding minsizes, i suppose the idea was to pass `ElementSizeInBytes` for
containers ? I am OK with the overshooting if you don't want to make this more
detailed, but can we drop the param?
================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:219
Reader R(Data);
size_t UncompressedSize = R.consume32();
if (R.err())
----------------
in theory, this could also trip us over. zlib::uncompress does a `reserve`
using this value. it is harder to come up with an upper bound here, but
https://zlib.net/zlib_tech.html#:~:text=Maximum%20Compression%20Factor&text=(The%20test%20case%20was%20a,sources)%20is%201032%3A1.
says the theoretical limit is 1032:1.
Maybe enforce that?
Also to make this more similar to others, it looks like `zlib::uncompress` also
has an overload taking a `const char *`.
================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:529
Reader CmdReader(Chunks.lookup("cmdl"));
if (CmdReader.err())
return error("malformed or truncated commandline section");
----------------
unrelated to this patch, but it feels like we are checking for the error at the
wrong place ?
================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:320
+ if (!Succeeded)
+ elog("Failed to set rlimit");
+ }
----------------
should we rather `ADD_FAILURE` ?
================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:345
+ std::string Serialized = llvm::to_string(Out);
+ llvm::consumeError(readIndexFile(Serialized).takeError());
+
----------------
why not `ASSERT_TRUE(readIndexFile(..))` ? (or `llvm::cantFail`)
================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:364
+ // The offset isn't trivial to find, so we use the file digest.
+ std::string FileDigest = llvm::fromHex("EED8F5EAF25C453C");
+ unsigned Pos = Srcs->Data.find_first_of(FileDigest);
----------------
can we use `In.Sources.begin()->Digest` instead?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91258/new/
https://reviews.llvm.org/D91258
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
