kadircet updated this revision to Diff 265181.
kadircet added a comment.
- Polish
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,9 +9,12 @@
#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 "clang/Format/Format.h"
#include "clang/Frontend/PrecompiledPreamble.h"
#include "clang/Lex/PreprocessorOptions.h"
#include "llvm/ADT/STLExtras.h"
@@ -198,7 +201,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 +246,122 @@
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");
+ }
+}
+
} // 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,9 +91,9 @@
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->DefRange.getBegin(), SM);
+ auto SID = getSymbolID(Macro->Name, Macro->IdentLoc, SM);
assert(SID);
EXPECT_THAT(ExpectedRefs,
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->DefRange.getBegin(),
- 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,14 +750,14 @@
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;
if (Macro) {
// Handle references to macro.
- if (auto MacroSID =
- getSymbolID(Macro->Name, Macro->DefRange.getBegin(), SM)) {
+ if (auto MacroSID = getSymbolID(Macro->Name, Macro->IdentLoc, SM)) {
// Collect macro references from main file.
const auto &IDToRefs = AST.getMacros().MacroRefs;
auto Refs = IDToRefs.find(*MacroSID);
@@ -871,12 +872,12 @@
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;
- if (!index::generateUSRForMacro(NewMacro.name, M->DefRange.getBegin(), SM,
- USR)) {
+ if (!index::generateUSRForMacro(NewMacro.name, M->IdentLoc, SM, USR)) {
NewMacro.USR = std::string(USR.str());
NewMacro.ID = SymbolID(NewMacro.USR);
}
Index: clang-tools-extra/clangd/SourceCode.h
===================================================================
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -291,12 +291,18 @@
struct DefinedMacro {
llvm::StringRef Name;
+ /// 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;
+ /// Full range of defintion, including the macro identifier. Isn't subject to
+ /// #line directive substitutions.
CharSourceRange DefRange;
};
/// 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 +312,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,13 +966,47 @@
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(),
- // MacroInfo::getDefinitionEndLoc returns the location for last token.
- CharSourceRange::getTokenRange(MI->getDefinitionLoc(),
- MI->getDefinitionEndLoc())};
- 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(), IdentLoc,
+ CharSourceRange::getTokenRange(
+ MI->getDefinitionLoc(), MI->getDefinitionEndLoc())};
}
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
@@ -555,7 +555,14 @@
// Try to get the full definition, not just the name
SourceLocation StartLoc = Macro.DefRange.getBegin();
SourceLocation EndLoc = Macro.DefRange.getEnd();
- 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 +713,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;
}
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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
