sammccall added a comment.
Code looks reasonable, but I don't understand why the changes are being made -
can you explain/link to a bug in the commit message?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:688
if (It != ExistingIncludes.end()) {
- Inc.Resolved = It->second.str();
+ // Copy everything from existing include, apart from the location, when
+ // it's coming from baseline preamble.
----------------
This doesn't seem related to the patch description, can you update it?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:690
+ // it's coming from baseline preamble.
+ if (It->second) {
+ Inc.Resolved = It->second->Resolved;
----------------
why the null check?
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:691
+ if (It->second) {
+ Inc.Resolved = It->second->Resolved;
+ Inc.HeaderID = It->second->HeaderID;
----------------
seems a bit more future-proof to copy the whole thing and just overwrite the
fields you want to preserve. (also would avoid the need for the comment)
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:708
+ // Includes haven't changed or we're not patching.
+ // So, copy them over from the baseline.
+ PP.PreambleIncludes = Baseline.Includes.MainFileIncludes;
----------------
this just echoes the code - say why instead?
(would also be good to have the reason in the commit message)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143197/new/
https://reviews.llvm.org/D143197
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
