kadircet updated this revision to Diff 265195.
kadircet added a comment.
- Rebase
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;
namespace clang {
namespace clangd {
@@ -198,7 +204,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);
}
@@ -243,6 +249,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->IdentLoc;
+ 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->IdentLoc, MainFilePath)) {
LocatedSymbol Macro;
Macro.Name = std::string(M->Name);
Macro.PreferredDeclaration = *Loc;
@@ -749,7 +750,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;
@@ -870,7 +872,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,15 @@
struct DefinedMacro {
llvm::StringRef Name;
const MacroInfo *Info;
+ /// Subject to #line directive substitution. E.g. a macro defined in a
+ /// built-in file might have an identifier location inside the main file.
+ SourceLocation IdentLoc;
};
/// 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 +310,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,45 @@
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 IdentLoc = 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(IdentLoc);
+ auto IncludeLoc = SM.getIncludeLoc(DefFile);
+ // Preamble patch is included inside the builtin file.
+ if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc)) {
+ auto Presumed = SM.getPresumedLoc(IdentLoc);
+ // Check that line directive is pointing at main file.
+ if (Presumed.isValid() && Presumed.getFileID().isInvalid() &&
+ isMainFile(Presumed.getFilename(), SM)) {
+ auto Presumed = SM.getPresumedLoc(IdentLoc);
+ // Now find the spelled token for macro identifier. The line looks like:
+ // # /*comment*/ define /*comment*/ MACRO_IDENT....
+ auto LocForStartOfLine =
+ SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(), 1);
+ auto SpelledToks = TB.spelledTokens(SM.getMainFileID());
+ auto *MacroIdent = llvm::partition_point(
+ SpelledToks, [&LocForStartOfLine](const syntax::Token &Tok) {
+ return Tok.location() < LocForStartOfLine;
+ });
+ assert(MacroIdent);
+ // This loop should terminate in a few iterations, unless something went
+ // really wrong.
+ while (MacroIdent < SpelledToks.end() &&
+ MacroIdent->text(SM) != SpelledTok.text(SM))
+ ++MacroIdent;
+
+ if (MacroIdent < SpelledToks.end())
+ IdentLoc = MacroIdent->location();
+ }
+ }
+
+ return DefinedMacro{IdentifierInfo->getName(), MI, IdentLoc};
}
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
@@ -423,8 +423,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
