sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Looks great, ship it! ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:154 + OS << "\\\n" << std::string(TargetColumn, ' '); + // Decrement because returned string has a line break before directive now. + --DirectiveLine; ---------------- nit: not "before the directive", just before the DirectiveRange.begin() ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:157 + } else { + // There is enough space for Prefix and space before directive, use it. + // We try to squeeze the Prefix into the same line whenever we can, as ---------------- nit: I think it'd be easier to understand the other case if this one came first, as it's the fallback ================ Comment at: clang-tools-extra/clangd/Preamble.h:136 +/// using presumed locations. Returns \p Loc if it isn't inside preamble patch. +SourceLocation translatePreamblePatchLocation(SourceLocation Loc, + const SourceManager &SM); ---------------- Can we have a unit-test for this function? I think the minimal thing (least entangled with PreamblePatch construction) is to manually write a snippet that looks like a preamble patch like ``` # line 3 main.cpp int foo(); ``` map it into a TestTU with AdditionalFiles, set `ExtraArgs = {"-include","patch.h"}` and then get the location with `findDecl` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80198/new/ https://reviews.llvm.org/D80198 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits