[PATCH] D143197: [clangd] Patch includes even without any changes

[Sam McCall via Phabricator via cfe-commits]

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