This is an archive of the discontinued LLVM Phabricator instance.

Differential D109880

[clangd] PreamblePatch should be no-op if includes arent patched
ClosedPublic

Authored by kadircet on Sep 16 2021, 6:44 AM.

Details

Reviewers
sammccall
Commits
rG64fe0458866d: [clangd] PreamblePatch should be no-op if includes arent patched
Summary

Don't create a useless functional patch with only filename in it when
there is only include directives to be patched but they're not
requested.

Diff Detail

Event Timeline

kadircet created this revision.Sep 16 2021, 6:44 AM
Herald added subscribers: usaxena95, arphaman. · View Herald TranscriptSep 16 2021, 6:44 AM
kadircet requested review of this revision.Sep 16 2021, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2021, 6:44 AM
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. · View Herald Transcript
Harbormaster completed remote builds in B124175: Diff 372920.Sep 16 2021, 7:26 AM
sammccall accepted this revision.Sep 16 2021, 7:29 AM
sammccall added inline comments.
clang-tools-extra/clangd/Preamble.cpp
462–463

I think =MacroDirectives would be significantly clearer than !=All, and a little less fragile

This revision is now accepted and ready to land.Sep 16 2021, 7:29 AM
Closed by commit rG64fe0458866d: [clangd] PreamblePatch should be no-op if includes arent patched (authored by kadircet). · Explain WhySep 16 2021, 8:12 AM
This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.
kadircet added a commit: rG64fe0458866d: [clangd] PreamblePatch should be no-op if includes arent patched.

Revision Contents

PathSize
clang-tools-extra/
clangd/
3 lines
unittests/
14 lines

Diff 372951

clang-tools-extra/clangd/Preamble.cpp

Show First 20 Lines • Show All 453 Lines • ▼ Show 20 Lines if (!ModifiedScan) {
elog("Failed to scan modified contents of {0}: {1}", FileName, elog("Failed to scan modified contents of {0}: {1}", FileName,
ModifiedScan.takeError()); ModifiedScan.takeError());
return PreamblePatch::unmodified(Baseline); return PreamblePatch::unmodified(Baseline);
} }
bool IncludesChanged = BaselineScan->Includes != ModifiedScan->Includes; bool IncludesChanged = BaselineScan->Includes != ModifiedScan->Includes;
bool DirectivesChanged = bool DirectivesChanged =
BaselineScan->TextualDirectives != ModifiedScan->TextualDirectives; BaselineScan->TextualDirectives != ModifiedScan->TextualDirectives;
if (!IncludesChanged && !DirectivesChanged) if ((PatchType == PatchType::MacroDirectives || !IncludesChanged) &&
!DirectivesChanged)
sammccallUnsubmitted
Done
ReplyInline Actions

I think =MacroDirectives would be significantly clearer than !=All, and a little less fragile

sammccall: I think `=MacroDirectives` would be significantly clearer than `!=All`, and a little less…
return PreamblePatch::unmodified(Baseline); return PreamblePatch::unmodified(Baseline);
PreamblePatch PP; PreamblePatch PP;
// This shouldn't coincide with any real file name. // This shouldn't coincide with any real file name.
llvm::SmallString<128> PatchName; llvm::SmallString<128> PatchName;
llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName), llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
PreamblePatchHeaderName); PreamblePatchHeaderName);
PP.PatchFileName = PatchName.str().str(); PP.PatchFileName = PatchName.str().str();
▲ Show 20 Lines • Show All 123 Lines • Show Last 20 Lines

clang-tools-extra/clangd/unittests/PreambleTests.cpp

Show First 20 Lines • Show All 548 Lines • ▼ Show 20 Lines
} }
TEST(PreamblePatch, MacroLoc) { TEST(PreamblePatch, MacroLoc) {
llvm::StringLiteral Baseline = "\n#define MACRO 12\nint num = MACRO;"; llvm::StringLiteral Baseline = "\n#define MACRO 12\nint num = MACRO;";
llvm::StringLiteral Modified = " \n#define MACRO 12\nint num = MACRO;"; llvm::StringLiteral Modified = " \n#define MACRO 12\nint num = MACRO;";
auto AST = createPatchedAST(Baseline, Modified); auto AST = createPatchedAST(Baseline, Modified);
ASSERT_TRUE(AST); ASSERT_TRUE(AST);
} }
TEST(PreamblePatch, NoopWhenNotRequested) {
llvm::StringLiteral Baseline = "#define M\nint num = M;";
llvm::StringLiteral Modified = "#define M\n#include <foo.h>\nint num = M;";
auto TU = TestTU::withCode(Baseline);
auto BaselinePreamble = TU.preamble();
ASSERT_TRUE(BaselinePreamble);
TU.Code = Modified.str();
MockFS FS;
auto PP = PreamblePatch::createMacroPatch(testPath(TU.Filename),
TU.inputs(FS), *BaselinePreamble);
EXPECT_TRUE(PP.text().empty());
}
} // namespace } // namespace
} // namespace clangd } // namespace clangd
} // namespace clang } // namespace clang