[PATCH] D42575: [clangd] Better handling symbols defined in macros.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL323867: [clangd] Better handling symbols defined in macros.
(authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D42575?vs=132123=132144#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42575
Files:
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/unittests/clangd/Annotations.cpp
clang-tools-extra/trunk/unittests/clangd/Annotations.h
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
Index: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
@@ -112,6 +112,37 @@
return false;
}
+// Return the symbol location of the given declaration `D`.
+//
+// For symbols defined inside macros:
+// * use expansion location, if the symbol is formed via macro concatenation.
+// * use spelling location, otherwise.
+SymbolLocation GetSymbolLocation(const NamedDecl *D, SourceManager ,
+ StringRef FallbackDir,
+ std::string ) {
+ SymbolLocation Location;
+
+ SourceLocation Loc = SM.getSpellingLoc(D->getLocation());
+ if (D->getLocation().isMacroID()) {
+// The symbol is formed via macro concatenation, the spelling location will
+// be "", which is not interesting to us, use the expansion
+// location instead.
+if (llvm::StringRef(Loc.printToString(SM)).startswith("getLocation())),
+ FallbackDir);
+ return {FilePathStorage,
+ SM.getFileOffset(SM.getExpansionRange(D->getLocStart()).first),
+ SM.getFileOffset(SM.getExpansionRange(D->getLocEnd()).second)};
+}
+ }
+
+ FilePathStorage = makeAbsolutePath(SM, SM.getFilename(Loc), FallbackDir);
+ return {FilePathStorage,
+ SM.getFileOffset(SM.getSpellingLoc(D->getLocStart())),
+ SM.getFileOffset(SM.getSpellingLoc(D->getLocEnd()))};
+}
+
} // namespace
SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
@@ -149,17 +180,14 @@
return true;
auto = ND->getASTContext().getSourceManager();
-std::string FilePath = makeAbsolutePath(
-SM, SM.getFilename(D->getLocation()), Opts.FallbackDir);
-SymbolLocation Location = {FilePath, SM.getFileOffset(D->getLocStart()),
- SM.getFileOffset(D->getLocEnd())};
std::string QName = ND->getQualifiedNameAsString();
Symbol S;
S.ID = std::move(ID);
std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
S.SymInfo = index::getSymbolInfo(D);
-S.CanonicalDeclaration = Location;
+std::string FilePath;
+S.CanonicalDeclaration = GetSymbolLocation(ND, SM, Opts.FallbackDir, FilePath);
// Add completion info.
assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
Index: clang-tools-extra/trunk/unittests/clangd/Annotations.h
===
--- clang-tools-extra/trunk/unittests/clangd/Annotations.h
+++ clang-tools-extra/trunk/unittests/clangd/Annotations.h
@@ -58,6 +58,10 @@
// Returns the location of all ranges marked by [[ ]] (or $name[[ ]]).
std::vector ranges(llvm::StringRef Name = "") const;
+ // The same to `range` method, but returns range in offsets [start, end).
+ std::pair
+ offsetRange(llvm::StringRef Name = "") const;
+
private:
std::string Code;
llvm::StringMap> Points;
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -7,6 +7,7 @@
//
//===--===//
+#include "Annotations.h"
#include "TestFS.h"
#include "index/SymbolCollector.h"
#include "index/SymbolYAML.h"
@@ -46,11 +47,22 @@
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(LocationOffsets, Offsets, "") {
+ // Offset range in SymbolLocation is [start, end] while in Clangd is [start,
+ // end).
+ return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
+ arg.CanonicalDeclaration.EndOffset == Offsets.second - 1;
+}
+//MATCHER_P(FilePath, P, "") {
+ //return arg.CanonicalDeclaration.FilePath.contains(P);
+//}
namespace clang {
namespace clangd {
namespace {
+const char TestHeaderName[] = "symbols.h";
+const
[PATCH] D42575: [clangd] Better handling symbols defined in macros.
hokein added inline comments.
Comment at: clangd/index/SymbolCollector.cpp:108
+std::string PrintableLoc(SourceLocation Loc, SourceManager ) {
+ if (Loc.isInvalid())
ioeric wrote:
> `PrintLoc`? `PrintableLoc` sounds like a checker for whether a location is
> printable.
`SourceLocation` has provided `printToString` API already, we don't need to
reinvent the wheel now.
Comment at: clangd/index/SymbolCollector.cpp:127
+// location instead.
+if (llvm::StringRef(PrintableLoc(Loc, SM)).startswith(" To reduce some code, consider
>
> ```
> Loc, Start, End = spelling locs;
> if (D->getLocation().isMacroID() && llvm::StringRef(PrintableLoc(Loc,
> SM)).startswith(" Loc, Start, End = expension locs;
> }
> // Generate SymbolLocation with Loc, Start, End
> ```
>
> Also, I think you could use `getLocStart` in place of `getLocation`?
There is no much duplicated code among the cases here (only one line of
generating the SymbolLocation). I prefer to the current way -- it shows the
logic a bit clearer, and also saves the cost of `getSpellingLoc` for the macro
case.
Comment at: unittests/clangd/Annotations.h:63
+ std::pair
+ offsetRange(llvm::StringRef Name = "") const;
+
ioeric wrote:
> This doesn't seem to be necessary? I think you could use the `range` method
> above and convert it to offsets in the test matcher.
I think this is a useful method (probably be used in the future). Putting it to
test matcher, we have to pass `Code` parameter to the matcher
(`LocationOffset(range, Header.code())`) everywhere. Annotation seems to be the
best place.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42575
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42575: [clangd] Better handling symbols defined in macros.
hokein updated this revision to Diff 132123.
hokein added a comment.
Update test.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42575
Files:
clangd/index/SymbolCollector.cpp
unittests/clangd/Annotations.cpp
unittests/clangd/Annotations.h
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -7,6 +7,7 @@
//
//===--===//
+#include "Annotations.h"
#include "index/SymbolCollector.h"
#include "index/SymbolYAML.h"
#include "clang/Basic/FileManager.h"
@@ -44,11 +45,22 @@
return arg.CompletionSnippetInsertText == S;
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
+MATCHER_P(LocationOffsets, Offsets, "") {
+ // Offset range in SymbolLocation is [start, end] while in Clangd is [start,
+ // end).
+ return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
+ arg.CanonicalDeclaration.EndOffset == Offsets.second - 1;
+}
+MATCHER_P(FilePath, P, "") {
+ return arg.CanonicalDeclaration.FilePath.contains(P);
+}
namespace clang {
namespace clangd {
namespace {
+const char TestHeaderName[] = "symbols.h";
+const char TestFileName[] = "symbol.cc";
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -77,21 +89,20 @@
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-const std::string FileName = "symbol.cc";
-const std::string HeaderName = "symbols.h";
auto Factory = llvm::make_unique(CollectorOpts);
tooling::ToolInvocation Invocation(
-{"symbol_collector", "-fsyntax-only", "-std=c++11", FileName},
+{"symbol_collector", "-fsyntax-only", "-std=c++11", TestFileName},
Factory->create(), Files.get(),
std::make_shared());
-InMemoryFileSystem->addFile(HeaderName, 0,
+InMemoryFileSystem->addFile(TestHeaderName, 0,
llvm::MemoryBuffer::getMemBuffer(HeaderCode));
-std::string Content = "#include\"" + std::string(HeaderName) + "\"";
-Content += "\n" + MainCode.str();
-InMemoryFileSystem->addFile(FileName, 0,
+std::string Content = MainCode;
+if (!HeaderCode.empty())
+ Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(Content));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
@@ -196,6 +207,57 @@
UnorderedElementsAre(QName("Foo")));
}
+TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
+ CollectorOpts.IndexMainFiles = false;
+
+ Annotations Header(R"(
+#define FF(name) \
+ class name##_Test {};
+
+$expansion[[FF(abc)]];
+
+#define FF2() \
+ $spelling[[class Test {}]];
+
+FF2();
+ )");
+
+ runSymbolCollector(Header.code(), /*Main=*/"");
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("abc_Test"),
+LocationOffsets(Header.offsetRange("expansion")),
+FilePath(TestHeaderName)),
+ AllOf(QName("Test"),
+LocationOffsets(Header.offsetRange("spelling")),
+FilePath(TestHeaderName;
+}
+
+TEST_F(SymbolCollectorTest, SymbolFormedFromMacroInMainFile) {
+ CollectorOpts.IndexMainFiles = true;
+
+ Annotations Main(R"(
+#define FF(name) \
+ class name##_Test {};
+
+$expansion[[FF(abc)]];
+
+#define FF2() \
+ $spelling[[class Test {}]];
+
+FF2();
+ )");
+ runSymbolCollector(/*Header=*/"", Main.code());
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("abc_Test"),
+LocationOffsets(Main.offsetRange("expansion")),
+FilePath(TestFileName)),
+ AllOf(QName("Test"),
+LocationOffsets(Main.offsetRange("spelling")),
+FilePath(TestFileName;
+}
+
TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
CollectorOpts.IndexMainFiles = false;
const std::string Header = R"(
Index: unittests/clangd/Annotations.h
===
--- unittests/clangd/Annotations.h
+++ unittests/clangd/Annotations.h
@@ -58,6 +58,10 @@
// Returns the location of all ranges marked by [[ ]] (or $name[[ ]]).
std::vector ranges(llvm::StringRef Name = "") const;
+ // The same to `range` method, but returns range in offsets [start, end).
+ std::pair
+ offsetRange(llvm::StringRef Name = "") const;
+
private:
std::string Code;
[PATCH] D42575: [clangd] Better handling symbols defined in macros.
hokein updated this revision to Diff 132122.
hokein marked 4 inline comments as done.
hokein added a comment.
- address review comments.
- add tests for indexing main files
- use SourceLocation.printToString
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42575
Files:
clangd/index/SymbolCollector.cpp
unittests/clangd/Annotations.cpp
unittests/clangd/Annotations.h
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -7,6 +7,7 @@
//
//===--===//
+#include "Annotations.h"
#include "index/SymbolCollector.h"
#include "index/SymbolYAML.h"
#include "clang/Basic/FileManager.h"
@@ -44,11 +45,22 @@
return arg.CompletionSnippetInsertText == S;
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
+MATCHER_P(LocationOffsets, Offsets, "") {
+ // Offset range in SymbolLocation is [start, end] while in Clangd is [start,
+ // end).
+ return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
+ arg.CanonicalDeclaration.EndOffset == Offsets.second - 1;
+}
+MATCHER_P(FilePath, P, "") {
+ return arg.CanonicalDeclaration.FilePath.contains(P);
+}
namespace clang {
namespace clangd {
namespace {
+const char TestHeaderName[] = "symbols.h";
+const char TestFileName[] = "symbol.cc";
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -77,21 +89,20 @@
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-const std::string FileName = "symbol.cc";
-const std::string HeaderName = "symbols.h";
auto Factory = llvm::make_unique(CollectorOpts);
tooling::ToolInvocation Invocation(
-{"symbol_collector", "-fsyntax-only", "-std=c++11", FileName},
+{"symbol_collector", "-fsyntax-only", "-std=c++11", TestFileName},
Factory->create(), Files.get(),
std::make_shared());
-InMemoryFileSystem->addFile(HeaderName, 0,
+InMemoryFileSystem->addFile(TestHeaderName, 0,
llvm::MemoryBuffer::getMemBuffer(HeaderCode));
-std::string Content = "#include\"" + std::string(HeaderName) + "\"";
-Content += "\n" + MainCode.str();
-InMemoryFileSystem->addFile(FileName, 0,
+std::string Content = MainCode;
+if (!HeaderCode.empty())
+ Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(Content));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
@@ -196,6 +207,57 @@
UnorderedElementsAre(QName("Foo")));
}
+TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
+ CollectorOpts.IndexMainFiles = false;
+
+ Annotations Header(R"(
+#define FF(name) \
+ class name##_Test {};
+
+$expansion[[FF(abc)]];
+
+#define FF2() \
+ $spelling[[class Test {}]];
+
+FF2();
+ )");
+
+ runSymbolCollector(Header.code(), /*Main=*/"");
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("abc_Test"),
+LocationOffsets(Header.offsetRange("expansion")),
+FilePath(TestHeaderName)),
+ AllOf(QName("Test"),
+LocationOffsets(Header.offsetRange("spelling")),
+FilePath(TestHeaderName;
+}
+
+TEST_F(SymbolCollectorTest, SymbolFormedFromMacroInMainFile) {
+ CollectorOpts.IndexMainFiles = true;
+
+ Annotations Header(R"(
+#define FF(name) \
+ class name##_Test {};
+
+$expansion[[FF(abc)]];
+
+#define FF2() \
+ $spelling[[class Test {}]];
+
+FF2();
+ )");
+ runSymbolCollector(/*Header=*/"", Header.code());
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("abc_Test"),
+LocationOffsets(Header.offsetRange("expansion")),
+FilePath(TestFileName)),
+ AllOf(QName("Test"),
+LocationOffsets(Header.offsetRange("spelling")),
+FilePath(TestFileName;
+}
+
TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
CollectorOpts.IndexMainFiles = false;
const std::string Header = R"(
Index: unittests/clangd/Annotations.h
===
--- unittests/clangd/Annotations.h
+++ unittests/clangd/Annotations.h
@@ -58,6 +58,10 @@
// Returns the location of all ranges marked by [[ ]] (or $name[[ ]]).
std::vector ranges(llvm::StringRef Name = "") const;
+ // The same to `range` method, but returns range in offsets
[PATCH] D42575: [clangd] Better handling symbols defined in macros.
ioeric added inline comments.
Comment at: clangd/index/SymbolCollector.cpp:108
+std::string PrintableLoc(SourceLocation Loc, SourceManager ) {
+ if (Loc.isInvalid())
`PrintLoc`? `PrintableLoc` sounds like a checker for whether a location is
printable.
Comment at: clangd/index/SymbolCollector.cpp:110
+ if (Loc.isInvalid())
+return "Invalid location";
+ std::string Buffer;
This works for the use case below (i.e. to get around "getLocation().isMacroID() && llvm::StringRef(PrintableLoc(Loc,
SM)).startswith("
+ offsetRange(llvm::StringRef Name = "") const;
+
This doesn't seem to be necessary? I think you could use the `range` method
above and convert it to offsets in the test matcher.
Comment at: unittests/clangd/SymbolCollectorTests.cpp:216-219
+$expansion[[FF(abc)]];
+
+#define FF2() \
+ $spelling[[class Test {}]];
Consider adding another expansion and spelling in the main file (with
`IndexMainFiles` enabled), so that we could also check file paths are handled
correctly?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42575
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42575: [clangd] Better handling symbols defined in macros.
hokein created this revision.
hokein added a reviewer: ioeric.
Herald added subscribers: jkorous-apple, ilya-biryukov, klimek.
For symbols defined inside macros:
- use expansion location, if the symbol is formed via macro concatenation.
- use spelling location, otherwise.
This will fix some symbols that have ill-format location (especial invalid
filepath).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42575
Files:
clangd/index/SymbolCollector.cpp
unittests/clangd/Annotations.cpp
unittests/clangd/Annotations.h
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -7,6 +7,7 @@
//
//===--===//
+#include "Annotations.h"
#include "index/SymbolCollector.h"
#include "index/SymbolYAML.h"
#include "clang/Basic/FileManager.h"
@@ -44,11 +45,22 @@
return arg.CompletionSnippetInsertText == S;
}
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
+MATCHER_P(LocationOffsets, Offsets, "") {
+ // Offset range in SymbolLocation is [start, end] while in Clangd is [start,
+ // end).
+ return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
+ arg.CanonicalDeclaration.EndOffset == Offsets.second - 1;
+}
+MATCHER_P(FilePath, P, "") {
+ return arg.CanonicalDeclaration.FilePath.contains(P);
+}
namespace clang {
namespace clangd {
namespace {
+const char TestHeaderName[] = "symbols.h";
+const char TestFileName[] = "symbol.cc";
class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
public:
SymbolIndexActionFactory(SymbolCollector::Options COpts)
@@ -77,21 +89,19 @@
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), InMemoryFileSystem));
-const std::string FileName = "symbol.cc";
-const std::string HeaderName = "symbols.h";
auto Factory = llvm::make_unique(CollectorOpts);
tooling::ToolInvocation Invocation(
-{"symbol_collector", "-fsyntax-only", "-std=c++11", FileName},
+{"symbol_collector", "-fsyntax-only", "-std=c++11", TestFileName},
Factory->create(), Files.get(),
std::make_shared());
-InMemoryFileSystem->addFile(HeaderName, 0,
+InMemoryFileSystem->addFile(TestHeaderName, 0,
llvm::MemoryBuffer::getMemBuffer(HeaderCode));
-std::string Content = "#include\"" + std::string(HeaderName) + "\"";
+std::string Content = "#include\"" + std::string(TestHeaderName) + "\"";
Content += "\n" + MainCode.str();
-InMemoryFileSystem->addFile(FileName, 0,
+InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(Content));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
@@ -196,6 +206,31 @@
UnorderedElementsAre(QName("Foo")));
}
+TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
+ CollectorOpts.IndexMainFiles = false;
+
+ Annotations Header(R"(
+#define FF(name) \
+ class name##_Test {};
+
+$expansion[[FF(abc)]];
+
+#define FF2() \
+ $spelling[[class Test {}]];
+
+FF2();
+ )");
+ runSymbolCollector(Header.code(), /*Main=*/"");
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("abc_Test"),
+LocationOffsets(Header.offsetRange("expansion")),
+FilePath(TestHeaderName)),
+ AllOf(QName("Test"),
+LocationOffsets(Header.offsetRange("spelling")),
+FilePath(TestHeaderName;
+}
+
TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
CollectorOpts.IndexMainFiles = false;
const std::string Header = R"(
Index: unittests/clangd/Annotations.h
===
--- unittests/clangd/Annotations.h
+++ unittests/clangd/Annotations.h
@@ -58,6 +58,10 @@
// Returns the location of all ranges marked by [[ ]] (or $name[[ ]]).
std::vector ranges(llvm::StringRef Name = "") const;
+ // Returns range in offsets [start, end).
+ std::pair
+ offsetRange(llvm::StringRef Name = "") const;
+
private:
std::string Code;
llvm::StringMap> Points;
Index: unittests/clangd/Annotations.cpp
===
--- unittests/clangd/Annotations.cpp
+++ unittests/clangd/Annotations.cpp
@@ -83,5 +83,11 @@
return {R.begin(), R.end()};
}
+std::pair
+Annotations::offsetRange(llvm::StringRef Name) const {
+ auto R = range(Name);
+ return {positionToOffset(Code, R.start), positionToOffset(Code, R.end)};
+}
+
} // namespace clangd
} // namespace clang
Index:
