sammccall marked 2 inline comments as done.
sammccall 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();
----------------
kadircet wrote:
> 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?
Yeah fair enough - I thought having the param made the logic easier to follow
but actually passing it made the code too fragile.
Just inlined badSize() and hardcoded MinSize=1, we can always extract it again
later.
================
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");
----------------
kadircet wrote:
> unrelated to this patch, but it feels like we are checking for the error at
> the wrong place ?
agree. Will fix in a different commit
================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:320
+ if (!Succeeded)
+ elog("Failed to set rlimit");
+ }
----------------
kadircet wrote:
> should we rather `ADD_FAILURE` ?
No, because rlimit can legitimately fail (e.g. we're already using more ram
than the limit we tried to set).
If rlimit fails then we may not be able to perform the test meaningfully, so we
could skip it in that case (i.e. decide to *pass* immediately).
================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:345
+ std::string Serialized = llvm::to_string(Out);
+ llvm::consumeError(readIndexFile(Serialized).takeError());
+
----------------
kadircet wrote:
> why not `ASSERT_TRUE(readIndexFile(..))` ? (or `llvm::cantFail`)
Whoops, this was leftover experimentation code.
================
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);
----------------
kadircet wrote:
> can we use `In.Sources.begin()->Digest` instead?
Hmm, we could, but it has the wrong data type (uint8_t vs char) so we need a
reinterpret_cast... not sure it's clearer.
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
