kadircet updated this revision to Diff 266111.
kadircet marked 10 inline comments as done.
kadircet added a comment.
- Extract preamble-patch location translation to a helper:
- Migrate usage in include collector
- Make preamble patch header name part of the check.
- Handle macro ids in spellDirective
- Tweak comments/names
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80198/new/
https://reviews.llvm.org/D80198
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -9,14 +9,19 @@
#include "Annotations.h"
#include "Compiler.h"
#include "Headers.h"
+#include "Hover.h"
#include "Preamble.h"
+#include "SourceCode.h"
#include "TestFS.h"
#include "TestTU.h"
+#include "XRefs.h"
+#include "clang/Format/Format.h"
#include "clang/Frontend/PrecompiledPreamble.h"
#include "clang/Lex/PreprocessorOptions.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "gmock/gmock.h"
@@ -28,6 +33,7 @@
using testing::Contains;
using testing::Field;
+using testing::Matcher;
using testing::MatchesRegex;
namespace clang {
@@ -199,7 +205,7 @@
ADD_FAILURE() << "Failed to build compiler invocation";
return llvm::None;
}
- return ParsedAST::build(testPath("main.cpp"), TU.inputs(), std::move(CI), {},
+ return ParsedAST::build(testPath(TU.Filename), TU.inputs(), std::move(CI), {},
BaselinePreamble);
}
@@ -228,7 +234,8 @@
#define BAR
[[BAR]])cpp",
R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
)cpp",
},
// multiline macro
@@ -238,7 +245,8 @@
[[BAR]])cpp",
R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
)cpp",
},
// multiline macro
@@ -248,7 +256,8 @@
BAR
[[BAR]])cpp",
R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 3
+#define BAR
)cpp",
},
};
@@ -275,8 +284,10 @@
)cpp");
llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
-#define BAR\(X, Y\) X Y
-#define BAR\(X\) X
+#line 2
+#define BAR\(X, Y\) X Y
+#line 3
+#define BAR\(X\) X
)cpp");
EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
MatchesRegex(ExpectedPatch.str()));
@@ -286,6 +297,169 @@
EXPECT_THAT(AST->getDiagnostics(),
Not(Contains(Field(&Diag::Range, Modified.range()))));
}
+
+TEST(PreamblePatchTest, LocateMacroAtWorks) {
+ struct {
+ llvm::StringLiteral Baseline;
+ llvm::StringLiteral Modified;
+ } Cases[] = {
+ // Addition of new directive
+ {
+ "",
+ R"cpp(
+ #define $def^FOO
+ $use^FOO)cpp",
+ },
+ // Available inside preamble section
+ {
+ "",
+ R"cpp(
+ #define $def^FOO
+ #undef $use^FOO)cpp",
+ },
+ // Available after undef, as we don't patch those
+ {
+ "",
+ R"cpp(
+ #define $def^FOO
+ #undef FOO
+ $use^FOO)cpp",
+ },
+ // Identifier on a different line
+ {
+ "",
+ R"cpp(
+ #define \
+ $def^FOO
+ $use^FOO)cpp",
+ },
+ // In presence of comment tokens
+ {
+ "",
+ R"cpp(
+ #\
+ define /* FOO */\
+ /* FOO */ $def^FOO
+ $use^FOO)cpp",
+ },
+ // Moved around
+ {
+ "#define FOO",
+ R"cpp(
+ #define BAR
+ #define $def^FOO
+ $use^FOO)cpp",
+ },
+ };
+ for (const auto &Case : Cases) {
+ SCOPED_TRACE(Case.Modified);
+ llvm::Annotations Modified(Case.Modified);
+ auto AST = createPatchedAST(Case.Baseline, Modified.code());
+ ASSERT_TRUE(AST);
+
+ const auto &SM = AST->getSourceManager();
+ auto *MacroTok = AST->getTokens().spelledTokenAt(
+ SM.getComposedLoc(SM.getMainFileID(), Modified.point("use")));
+ ASSERT_TRUE(MacroTok);
+
+ auto FoundMacro = locateMacroAt(*MacroTok, AST->getPreprocessor());
+ ASSERT_TRUE(FoundMacro);
+ EXPECT_THAT(FoundMacro->Name, "FOO");
+
+ auto MacroLoc = FoundMacro->NameLoc;
+ EXPECT_EQ(SM.getFileID(MacroLoc), SM.getMainFileID());
+ EXPECT_EQ(SM.getFileOffset(MacroLoc), Modified.point("def"));
+ }
+}
+
+TEST(PreamblePatchTest, LocateMacroAtDeletion) {
+ {
+ // We don't patch deleted define directives, make sure we don't crash.
+ llvm::StringLiteral Baseline = "#define FOO";
+ llvm::Annotations Modified("^FOO");
+
+ auto AST = createPatchedAST(Baseline, Modified.code());
+ ASSERT_TRUE(AST);
+
+ const auto &SM = AST->getSourceManager();
+ auto *MacroTok = AST->getTokens().spelledTokenAt(
+ SM.getComposedLoc(SM.getMainFileID(), Modified.point()));
+ ASSERT_TRUE(MacroTok);
+
+ auto FoundMacro = locateMacroAt(*MacroTok, AST->getPreprocessor());
+ ASSERT_TRUE(FoundMacro);
+ EXPECT_THAT(FoundMacro->Name, "FOO");
+ auto HI =
+ getHover(*AST, offsetToPosition(Modified.code(), Modified.point()),
+ format::getLLVMStyle(), nullptr);
+ ASSERT_TRUE(HI);
+ EXPECT_THAT(HI->Definition, testing::IsEmpty());
+ }
+
+ {
+ // Offset is valid, but underlying text is different.
+ llvm::StringLiteral Baseline = "#define FOO";
+ Annotations Modified(R"cpp(#define BAR
+ ^FOO")cpp");
+
+ auto AST = createPatchedAST(Baseline, Modified.code());
+ ASSERT_TRUE(AST);
+
+ auto HI = getHover(*AST, Modified.point(), format::getLLVMStyle(), nullptr);
+ ASSERT_TRUE(HI);
+ EXPECT_THAT(HI->Definition, "#define BAR");
+ }
+}
+
+TEST(PreamblePatchTest, RefsToMacros) {
+ struct {
+ llvm::StringLiteral Baseline;
+ llvm::StringLiteral Modified;
+ } Cases[] = {
+ // Newly added
+ {
+ "",
+ R"cpp(
+ #define ^FOO
+ ^[[FOO]])cpp",
+ },
+ // Moved around
+ {
+ "#define FOO",
+ R"cpp(
+ #define BAR
+ #define ^FOO
+ ^[[FOO]])cpp",
+ },
+ // Ref in preamble section
+ {
+ "",
+ R"cpp(
+ #define ^FOO
+ #undef ^FOO)cpp",
+ },
+ };
+
+ for (const auto &Case : Cases) {
+ Annotations Modified(Case.Modified);
+ auto AST = createPatchedAST("", Modified.code());
+ ASSERT_TRUE(AST);
+
+ const auto &SM = AST->getSourceManager();
+ std::vector<Matcher<Location>> ExpectedLocations;
+ for (const auto &R : Modified.ranges())
+ ExpectedLocations.push_back(Field(&Location::range, R));
+
+ for (const auto &P : Modified.points()) {
+ auto *MacroTok = AST->getTokens().spelledTokenAt(SM.getComposedLoc(
+ SM.getMainFileID(),
+ llvm::cantFail(positionToOffset(Modified.code(), P))));
+ ASSERT_TRUE(MacroTok);
+ EXPECT_THAT(findReferences(*AST, P, 0).References,
+ testing::ElementsAreArray(ExpectedLocations));
+ }
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -306,7 +306,7 @@
}
TEST_F(HeadersTest, PresumedLocations) {
- std::string HeaderFile = "implicit_include.h";
+ std::string HeaderFile = "__preamble_patch__.h";
// Line map inclusion back to main file.
std::string HeaderContents =
@@ -317,7 +317,7 @@
FS.Files[HeaderFile] = HeaderContents;
// Including through non-builtin file has no effects.
- FS.Files[MainFile] = "#include \"implicit_include.h\"\n\n";
+ FS.Files[MainFile] = "#include \"__preamble_patch__.h\"\n\n";
EXPECT_THAT(collectIncludes().MainFileIncludes,
Not(Contains(Written("<a.h>"))));
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -206,8 +206,8 @@
locateMacroReferent(const syntax::Token &TouchedIdentifier, ParsedAST &AST,
llvm::StringRef MainFilePath) {
if (auto M = locateMacroAt(TouchedIdentifier, AST.getPreprocessor())) {
- if (auto Loc = makeLocation(AST.getASTContext(),
- M->Info->getDefinitionLoc(), MainFilePath)) {
+ if (auto Loc =
+ makeLocation(AST.getASTContext(), M->NameLoc, MainFilePath)) {
LocatedSymbol Macro;
Macro.Name = std::string(M->Name);
Macro.PreferredDeclaration = *Loc;
Index: clang-tools-extra/clangd/SourceCode.h
===================================================================
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -292,6 +292,10 @@
struct DefinedMacro {
llvm::StringRef Name;
const MacroInfo *Info;
+ /// Location of the identifier that names the macro.
+ /// Unlike Info->Location, this translates preamble-patch locations to
+ /// main-file locations.
+ SourceLocation NameLoc;
};
/// Gets the macro referenced by \p SpelledTok. It must be a spelled token
/// aligned to the beginning of an identifier.
Index: clang-tools-extra/clangd/SourceCode.cpp
===================================================================
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -8,6 +8,7 @@
#include "SourceCode.h"
#include "FuzzyMatch.h"
+#include "Preamble.h"
#include "Protocol.h"
#include "refactor/Tweak.h"
#include "support/Context.h"
@@ -961,7 +962,9 @@
Loc = Loc.getLocWithOffset(-1);
MacroDefinition MacroDef = PP.getMacroDefinitionAtLoc(IdentifierInfo, Loc);
if (auto *MI = MacroDef.getMacroInfo())
- return DefinedMacro{IdentifierInfo->getName(), MI};
+ return DefinedMacro{
+ IdentifierInfo->getName(), MI,
+ translatePreamblePatchLocation(MI->getDefinitionLoc(), SM)};
return None;
}
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -131,6 +131,11 @@
std::vector<Inclusion> PreambleIncludes;
};
+/// Translates locations inside preamble patch to their main-file equivalent
+/// using presumed locations. Returns \p Loc if it isn't inside preamble patch.
+SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
+ const SourceManager &SM);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -50,6 +50,7 @@
namespace clang {
namespace clangd {
namespace {
+constexpr llvm::StringLiteral PreamblePatchHeaderName = "__preamble_patch__.h";
bool compileCommandsAreEqual(const tooling::CompileCommand &LHS,
const tooling::CompileCommand &RHS) {
@@ -120,6 +121,48 @@
}
};
+// Formats a PP directive consisting of Prefix (e.g. "#define ") and Body ("X
+// 10"). The formatting is copied so that the tokens in Body have PresumedLocs
+// with correct columns and lines.
+std::string spellDirective(llvm::StringRef Prefix,
+ CharSourceRange DirectiveRange,
+ const LangOptions &LangOpts, const SourceManager &SM,
+ unsigned &DirectiveLine) {
+ std::string SpelledDirective;
+ llvm::raw_string_ostream OS(SpelledDirective);
+ OS << Prefix;
+
+ // Make sure DirectiveRange is a char range and doesn't contain macro ids.
+ DirectiveRange = SM.getExpansionRange(DirectiveRange);
+ if (DirectiveRange.isTokenRange()) {
+ DirectiveRange.setEnd(
+ Lexer::getLocForEndOfToken(DirectiveRange.getEnd(), 0, SM, LangOpts));
+ }
+
+ auto DecompLoc = SM.getDecomposedLoc(DirectiveRange.getBegin());
+ DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+ auto TargetColumn = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1;
+
+ // Pad with spaces before DirectiveRange to make sure it will be on right
+ // column when patched.
+ if (TargetColumn < Prefix.size()) {
+ // Prefix was longer than the space we had. We produce e.g.:
+ // #line N-1
+ // #define \
+ // X 10
+ OS << "\\\n" << std::string(TargetColumn, ' ');
+ // Decrement because returned string has a line break before directive now.
+ --DirectiveLine;
+ } 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
+ // putting onto a separate line won't work at the beginning of the file.
+ OS << std::string(TargetColumn - Prefix.size(), ' ');
+ }
+ OS << toSourceCode(SM, DirectiveRange.getAsRange());
+ return OS.str();
+}
+
// Collects #define directives inside the main file.
struct DirectiveCollector : public PPCallbacks {
DirectiveCollector(const Preprocessor &PP,
@@ -140,15 +183,12 @@
TextualDirectives.emplace_back();
TextualPPDirective &TD = TextualDirectives.back();
- auto DecompLoc = SM.getDecomposedLoc(MacroNameTok.getLocation());
- TD.DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
-
- SourceRange DefRange(
- MD->getMacroInfo()->getDefinitionLoc(),
- Lexer::getLocForEndOfToken(MD->getMacroInfo()->getDefinitionEndLoc(), 0,
- SM, LangOpts));
- llvm::raw_string_ostream OS(TD.Text);
- OS << "#define " << toSourceCode(SM, DefRange);
+ const auto *MI = MD->getMacroInfo();
+ TD.Text =
+ spellDirective("#define ",
+ CharSourceRange::getTokenRange(
+ MI->getDefinitionLoc(), MI->getDefinitionEndLoc()),
+ LangOpts, SM, TD.DirectiveLine);
}
private:
@@ -231,6 +271,13 @@
}
llvm_unreachable("not an include directive");
}
+
+// Checks whether \p FileName is a valid spelling of main file.
+bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) {
+ auto FE = SM.getFileManager().getFile(FileName);
+ return FE && *FE == SM.getFileEntryForID(SM.getMainFileID());
+}
+
} // namespace
PreambleData::PreambleData(const ParseInputs &Inputs,
@@ -374,7 +421,7 @@
// This shouldn't coincide with any real file name.
llvm::SmallString<128> PatchName;
llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
- "__preamble_patch__.h");
+ PreamblePatchHeaderName);
PP.PatchFileName = PatchName.str().str();
llvm::raw_string_ostream Patch(PP.PatchContents);
@@ -428,8 +475,10 @@
// Note that we deliberately ignore conditional directives and undefs to
// reduce complexity. The former might cause problems because scanning is
// imprecise and might pick directives from disabled regions.
- for (const auto &TD : ModifiedScan->TextualDirectives)
+ for (const auto &TD : ModifiedScan->TextualDirectives) {
+ Patch << "#line " << TD.DirectiveLine << '\n';
Patch << TD.Text << '\n';
+ }
}
dlog("Created preamble patch: {0}", Patch.str());
Patch.flush();
@@ -462,5 +511,24 @@
return PP;
}
+SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
+ const SourceManager &SM) {
+ auto DefFile = SM.getFileID(Loc);
+ if (auto *FE = SM.getFileEntryForID(DefFile)) {
+ auto IncludeLoc = SM.getIncludeLoc(DefFile);
+ // Preamble patch is included inside the builtin file.
+ if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc) &&
+ FE->getName().endswith(PreamblePatchHeaderName)) {
+ auto Presumed = SM.getPresumedLoc(Loc);
+ // Check that line directive is pointing at main file.
+ if (Presumed.isValid() && Presumed.getFileID().isInvalid() &&
+ isMainFile(Presumed.getFilename(), SM)) {
+ Loc = SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(),
+ Presumed.getColumn());
+ }
+ }
+ }
+ return Loc;
+}
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Hover.cpp
===================================================================
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -376,8 +376,7 @@
const auto *ME = llvm::dyn_cast<MemberExpr>(E->IgnoreCasts());
if (!ME || !llvm::isa<CXXThisExpr>(ME->getBase()->IgnoreCasts()))
return llvm::None;
- const auto *Field =
- llvm::dyn_cast<FieldDecl>(ME->getMemberDecl());
+ const auto *Field = llvm::dyn_cast<FieldDecl>(ME->getMemberDecl());
if (!Field || !Field->getDeclName().isIdentifier())
return llvm::None;
return Field->getDeclName().getAsIdentifierInfo()->getName();
@@ -555,7 +554,14 @@
// Try to get the full definition, not just the name
SourceLocation StartLoc = Macro.Info->getDefinitionLoc();
SourceLocation EndLoc = Macro.Info->getDefinitionEndLoc();
- if (EndLoc.isValid()) {
+ // Ensure that EndLoc is a valid offset. For example it might come from
+ // preamble, and source file might've changed, in such a scenario EndLoc still
+ // stays valid, but getLocForEndOfToken will fail as it is no longer a valid
+ // offset.
+ // Note that this check is just to ensure there's text data inside the range.
+ // It will still succeed even when the data inside the range is irrelevant to
+ // macro definition.
+ if (SM.getPresumedLoc(EndLoc, /*UseLineDirectives=*/false).isValid()) {
EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM, AST.getLangOpts());
bool Invalid;
StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
@@ -869,20 +875,20 @@
if (!Suffix.empty() && !AfterEndChars.contains(Suffix.front()))
return llvm::None;
- return Line.slice(Offset, Next+1);
+ return Line.slice(Offset, Next + 1);
}
void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph &Out) {
// Probably this is appendText(Line), but scan for something interesting.
for (unsigned I = 0; I < Line.size(); ++I) {
switch (Line[I]) {
- case '`':
- if (auto Range = getBacktickQuoteRange(Line, I)) {
- Out.appendText(Line.substr(0, I));
- Out.appendCode(Range->trim("`"), /*Preserve=*/true);
- return parseDocumentationLine(Line.substr(I+Range->size()), Out);
- }
- break;
+ case '`':
+ if (auto Range = getBacktickQuoteRange(Line, I)) {
+ Out.appendText(Line.substr(0, I));
+ Out.appendCode(Range->trim("`"), /*Preserve=*/true);
+ return parseDocumentationLine(Line.substr(I + Range->size()), Out);
+ }
+ break;
}
}
Out.appendText(Line).appendSpace();
Index: clang-tools-extra/clangd/Headers.cpp
===================================================================
--- clang-tools-extra/clangd/Headers.cpp
+++ clang-tools-extra/clangd/Headers.cpp
@@ -8,6 +8,7 @@
#include "Headers.h"
#include "Compiler.h"
+#include "Preamble.h"
#include "SourceCode.h"
#include "support/Logger.h"
#include "clang/Basic/SourceLocation.h"
@@ -23,11 +24,6 @@
namespace clangd {
namespace {
-bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) {
- auto FE = SM.getFileManager().getFile(FileName);
- return FE && *FE == SM.getFileEntryForID(SM.getMainFileID());
-}
-
class RecordHeaders : public PPCallbacks {
public:
RecordHeaders(const SourceManager &SM, IncludeStructure *Out)
@@ -44,17 +40,8 @@
SrcMgr::CharacteristicKind FileKind) override {
auto MainFID = SM.getMainFileID();
// If an include is part of the preamble patch, translate #line directives.
- if (InBuiltinFile) {
- auto Presumed = SM.getPresumedLoc(HashLoc);
- // Presumed locations will have an invalid file id when #line directive
- // changes the filename.
- if (Presumed.getFileID().isInvalid() &&
- isMainFile(Presumed.getFilename(), SM)) {
- // Now we'll hit the case below.
- HashLoc = SM.translateLineCol(MainFID, Presumed.getLine(),
- Presumed.getColumn());
- }
- }
+ if (InBuiltinFile)
+ HashLoc = translatePreamblePatchLocation(HashLoc, SM);
// Record main-file inclusions (including those mapped from the preamble
// patch).
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
