[PATCH] D78743: [clangd] Preserve line information while build PreamblePatch

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:321
   llvm::raw_string_ostream Patch(PP.PatchContents);
+  Patch << llvm::formatv("#line 0 \"{0}\"\n", FileName);
   for (const auto &Inc : *ModifiedIncludes) {
----------------
what's the purpose of this? to avoid repeating the filename everywhere in the patch? maybe add a comment "// set default filename for subsequent #line directives"


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:327
+    // #line to set the presumed location to where it is spelled
+    // FIXME: Traverse once.
+    auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset);
----------------
I doubt we'd ever bother fixing this.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:331
+    llvm::StringRef Directive = spellingForIncDirective(Inc.Directive);
+    size_t Padding = Inc.R.start.character - Directive.size() - LineCol.second;
+    Patch << llvm::formatv("{0}#{1}{2}{3}\n",
----------------
(this needs an update for the deleting-R patch)


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:139
 TEST(PreamblePatchTest, ContainsNewIncludes) {
-  MockFSProvider FS;
-  MockCompilationDatabase CDB;
-  IgnoreDiagnostics Diags;
-  // ms-compatibility changes meaning of #import, make sure it is turned off.
-  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
-
-  const auto FileName = testPath("foo.cc");
-  ParseInputs PI;
-  PI.FS = FS.getFileSystem();
-  PI.CompileCommand = *CDB.getCompileCommand(FileName);
-  PI.Contents = "#include <existing.h>\n";
-
-  // Check diffing logic by adding a new header to the preamble and ensuring
-  // only it is patched.
-  const auto CI = buildCompilerInvocation(PI, Diags);
-  const auto FullPreamble = buildPreamble(FileName, *CI, PI, true, nullptr);
-
+  constexpr llvm::StringLiteral BaseLineContents = "#include <existing.h>\n";
   constexpr llvm::StringLiteral Patch =
----------------
nit: baseline is one word


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:142
       "#include <test>\n#import <existing.h>\n";
   // We provide the same includes twice, they shouldn't be included in the
   // patch.
----------------
I missed this last time, but this test seems unneccesarily confusing
 - can we just out the baseline and modified preambles instead of concatenating things together?
 - duplicated headers is worth testing but hardly the only case. Can we have a removed header as well, as one that was moved but not duplicated?
 - I think it'd be worth asserting the line numbers too



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78743/new/

https://reviews.llvm.org/D78743