kadircet updated this revision to Diff 266047.
kadircet marked 3 inline comments as done.
kadircet added a comment.
Instead of re-lexing at use time, put macro identifier on correct column while
patching and make use of presumed locations at use time.
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/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -514,7 +514,7 @@
ASSERT_TRUE(bool(CurLoc));
const auto *Id = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
ASSERT_TRUE(Id);
- auto Result = locateMacroAt(*Id, AST.getPreprocessor());
+ auto Result = locateMacroAt(*Id, AST.getPreprocessor(), AST.getTokens());
ASSERT_TRUE(Result);
EXPECT_THAT(*Result, MacroName("MACRO"));
}
@@ -528,7 +528,7 @@
ASSERT_TRUE(bool(CurLoc));
const auto *Id = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
ASSERT_TRUE(Id);
- auto Result = locateMacroAt(*Id, AST.getPreprocessor());
+ auto Result = locateMacroAt(*Id, AST.getPreprocessor(), AST.getTokens());
ASSERT_TRUE(Result);
EXPECT_THAT(*Result, MacroName("MACRO"));
}
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,171 @@
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(), AST->getTokens());
+ 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(), AST->getTokens());
+ 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/CollectMacrosTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp
+++ clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp
@@ -91,7 +91,7 @@
ASSERT_TRUE(bool(Loc));
const auto *Id = syntax::spelledIdentifierTouching(*Loc, AST.getTokens());
ASSERT_TRUE(Id);
- auto Macro = locateMacroAt(*Id, PP);
+ auto Macro = locateMacroAt(*Id, PP, AST.getTokens());
assert(Macro);
auto SID = getSymbolID(Macro->Name, Macro->Info, SM);
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -477,7 +477,7 @@
return makeError(ReasonToReject::NoSymbolFound);
// FIXME: Renaming macros is not supported yet, the macro-handling code should
// be moved to rename tooling library.
- if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
+ if (locateMacroAt(*IdentifierToken, AST.getPreprocessor(), AST.getTokens()))
return makeError(ReasonToReject::UnsupportedSymbol);
auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -205,9 +205,10 @@
llvm::Optional<LocatedSymbol>
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 M = locateMacroAt(TouchedIdentifier, AST.getPreprocessor(),
+ AST.getTokens())) {
+ if (auto Loc =
+ makeLocation(AST.getASTContext(), M->NameLoc, MainFilePath)) {
LocatedSymbol Macro;
Macro.Name = std::string(M->Name);
Macro.PreferredDeclaration = *Loc;
@@ -765,7 +766,8 @@
llvm::Optional<DefinedMacro> Macro;
if (const auto *IdentifierAtCursor =
syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens())) {
- Macro = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor());
+ Macro = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor(),
+ AST.getTokens());
}
RefsRequest Req;
@@ -886,7 +888,8 @@
if (!IdentifierAtCursor)
return Results;
- if (auto M = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor())) {
+ if (auto M = locateMacroAt(*IdentifierAtCursor, AST.getPreprocessor(),
+ AST.getTokens())) {
SymbolDetails NewMacro;
NewMacro.name = std::string(M->Name);
llvm::SmallString<32> USR;
Index: clang-tools-extra/clangd/SourceCode.h
===================================================================
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -292,11 +292,16 @@
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.
llvm::Optional<DefinedMacro> locateMacroAt(const syntax::Token &SpelledTok,
- Preprocessor &PP);
+ Preprocessor &PP,
+ const syntax::TokenBuffer &TB);
/// Infers whether this is a header from the FileName and LangOpts (if
/// presents).
@@ -306,6 +311,9 @@
/// Returns true if the given location is in a generated protobuf file.
bool isProtoFile(SourceLocation Loc, const SourceManager &SourceMgr);
+/// Checks whether \p FileName is a valid spelling of main file.
+bool isMainFile(llvm::StringRef FileName, const SourceManager &SM);
+
} // namespace clangd
} // namespace clang
#endif
Index: clang-tools-extra/clangd/SourceCode.cpp
===================================================================
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -945,8 +945,14 @@
return Result;
}
+bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) {
+ auto FE = SM.getFileManager().getFile(FileName);
+ return FE && *FE == SM.getFileEntryForID(SM.getMainFileID());
+}
+
llvm::Optional<DefinedMacro> locateMacroAt(const syntax::Token &SpelledTok,
- Preprocessor &PP) {
+ Preprocessor &PP,
+ const syntax::TokenBuffer &TB) {
SourceLocation Loc = SpelledTok.location();
assert(Loc.isFileID());
const auto &SM = PP.getSourceManager();
@@ -960,9 +966,27 @@
if (SM.getLocForStartOfFile(SM.getFileID(Loc)) != Loc)
Loc = Loc.getLocWithOffset(-1);
MacroDefinition MacroDef = PP.getMacroDefinitionAtLoc(IdentifierInfo, Loc);
- if (auto *MI = MacroDef.getMacroInfo())
- return DefinedMacro{IdentifierInfo->getName(), MI};
- return None;
+ auto *MI = MacroDef.getMacroInfo();
+ if (!MI)
+ return None;
+
+ SourceLocation NameLoc = MI->getDefinitionLoc();
+ // Macro definitions could be injected through preamble patch. These contain
+ // #line directives to hint their original location in main file.
+ auto DefFile = SM.getFileID(NameLoc);
+ auto IncludeLoc = SM.getIncludeLoc(DefFile);
+ // Preamble patch is included inside the builtin file.
+ if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc)) {
+ auto Presumed = SM.getPresumedLoc(NameLoc);
+ // Check that line directive is pointing at main file.
+ if (Presumed.isValid() && Presumed.getFileID().isInvalid() &&
+ isMainFile(Presumed.getFilename(), SM)) {
+ NameLoc = SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(),
+ Presumed.getColumn());
+ }
+ }
+
+ return DefinedMacro{IdentifierInfo->getName(), MI, NameLoc};
}
llvm::Expected<std::string> Edit::apply() const {
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -120,6 +120,36 @@
}
};
+// Spells directive in \p DirectiveRange while prepending it with \p Prefix.
+// Returned string can be used with a #line directive on line \p DirectiveLine
+// to correctly align with \p DirectiveRange.
+std::string spellDirective(llvm::StringRef Prefix, SourceRange DirectiveRange,
+ unsigned &DirectiveLine, const SourceManager &SM) {
+ std::string SpelledDirective;
+ llvm::raw_string_ostream OS(SpelledDirective);
+ OS << Prefix;
+
+ auto DecompLoc = SM.getDecomposedLoc(DirectiveRange.getBegin());
+ DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+ auto SpaceBefore = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1;
+
+ // Pad with spaces before DirectiveRange to make sure it will be on right
+ // column when patched.
+ if (SpaceBefore < Prefix.size()) {
+ // Prefix was longer than the space we had. Pad from start of a new line.
+ OS << "\\\n" << std::string(SpaceBefore, ' ');
+ // 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(SpaceBefore - Prefix.size(), ' ');
+ }
+ OS << toSourceCode(SM, DirectiveRange);
+ return OS.str();
+}
+
// Collects #define directives inside the main file.
struct DirectiveCollector : public PPCallbacks {
DirectiveCollector(const Preprocessor &PP,
@@ -140,15 +170,11 @@
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);
+ TD.Text = spellDirective("#define ", DefRange, TD.DirectiveLine, SM);
}
private:
@@ -425,8 +451,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();
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);
@@ -706,7 +712,7 @@
if (Tok.kind() == tok::identifier) {
// Prefer the identifier token as a fallback highlighting range.
HighlightRange = Tok.range(SM).toCharRange(SM);
- if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) {
+ if (auto M = locateMacroAt(Tok, AST.getPreprocessor(), AST.getTokens())) {
HI = getHoverContents(*M, AST);
break;
}
@@ -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
@@ -23,11 +23,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)
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
