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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
