[PATCH] D38639: [clangd] #include statements support for Open definition
Marc-Andre Laperle via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 14 21:03:23 PST 2017
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
================
Comment at: clangd/ClangdServer.cpp:454
+
+ IncludeReferenceMap IRM = std::move(AST->takeIRM());
+ Result = clangd::findDefinitions(*AST, Pos, Logger, IRM.IncludeLocationMap);
----------------
IncludeReferenceMap & here? See other comment in takeIRM
================
Comment at: clangd/ClangdUnit.cpp:34
#include <chrono>
+#include <iostream>
----------------
remove
================
Comment at: clangd/ClangdUnit.cpp:77
-class CppFilePreambleCallbacks : public PreambleCallbacks {
+void fillRangeVector(
+ const SourceManager &SM,
----------------
remove
================
Comment at: clangd/ClangdUnit.cpp:96
+
+void findPreambleIncludes(
+ const SourceManager &SM,
----------------
remove
================
Comment at: clangd/ClangdUnit.cpp:118
+ CppFilePreambleCallbacks(SourceManager *SourceMgr, IncludeReferenceMap &IRM)
+ : SourceMgr(SourceMgr), IRM(IRM) {}
+
----------------
These need to be swapped to be in the same order that they are declared below.
================
Comment at: clangd/ClangdUnit.cpp:120
+
+ IncludeReferenceMap takeIRM() {
+ fillRangeVector(*SourceMgr, IRM.DataVector, IRM.RangeVector);
----------------
I don't think we need this if we pass the map by reference (and store it as a reference, see other comment)
================
Comment at: clangd/ClangdUnit.cpp:127
+
+ IncludeReferenceMap getIRM() { return IRM; };
+
----------------
Remove (see previous comment)
================
Comment at: clangd/ClangdUnit.cpp:155
+ }
+
+ void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
----------------
I tried to simply the methods introduced here:
```
void AfterExecute(CompilerInstance &CI) override {
SourceMgr = &CI.getSourceManager();
for (auto InclusionLoc : TempPreambleIncludeLocations)
addIncludeLocation(InclusionLoc);
}
void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
StringRef FileName, bool IsAngled,
CharSourceRange FilenameRange, const FileEntry *File,
StringRef SearchPath, StringRef RelativePath,
const Module *Imported) override {
auto SR = FilenameRange.getAsRange();
if (SR.isInvalid() || !File || File->tryGetRealPathName().empty())
return;
if (SourceMgr) {
addIncludeLocation({SR, File->tryGetRealPathName()});
} else {
// When we are processing the inclusion directives inside the preamble,
// we don't have access to a SourceManager, so we cannot convert
// SourceRange to Range. This is done instead in AfterExecute when a
// SourceManager becomes available.
TempPreambleIncludeLocations.push_back({SR, File->tryGetRealPathName()});
}
}
void addIncludeLocation(std::pair<SourceRange, std::string> InclusionLoc) {
// Only inclusion directives in the main file make sense. The user cannot
// select directives not in the main file.
if (SourceMgr->getMainFileID() == SourceMgr->getFileID(InclusionLoc.first.getBegin()))
IRM.insert({getRange(InclusionLoc.first), InclusionLoc.second});
}
Range getRange(SourceRange SR) {
Position Begin;
Begin.line = SourceMgr->getSpellingLineNumber(SR.getBegin());
Begin.character = SourceMgr->getSpellingColumnNumber(SR.getBegin());
Position End;
End.line = SourceMgr->getSpellingLineNumber(SR.getEnd());
End.character = SourceMgr->getSpellingColumnNumber(SR.getEnd());
return {Begin, End};
}
```
================
Comment at: clangd/ClangdUnit.cpp:177
+ Range R = {Begin, End};
+ if (File && !File->tryGetRealPathName().empty())
+ IRM.IncludeLocationMap.insert({R, File->tryGetRealPathName()});
----------------
I think we need a "is main file" check here. In case this is used on a AST with no preamble.
================
Comment at: clangd/ClangdUnit.cpp:209
std::vector<serialization::DeclID> TopLevelDeclIDs;
+ IncludeReferenceMap IRM;
+ SourceManager *SourceMgr;
----------------
IncludeReferenceMap &IRM;
That way, we can use the same map and pass it multiple times to different "collectors".
================
Comment at: clangd/ClangdUnit.cpp:266
+class EmptyDiagsConsumer : public DiagnosticConsumer {
+public:
----------------
I don't think this change was brought back intentionally?
================
Comment at: clangd/ClangdUnit.cpp:288
IntrusiveRefCntPtr<vfs::FileSystem> VFS,
- clangd::Logger &Logger) {
+ clangd::Logger &Logger, IncludeReferenceMap IRM) {
----------------
IncludeReferenceMap &IRM
================
Comment at: clangd/ClangdUnit.cpp:314
+
+ Clang->getPreprocessor().addPPCallbacks(std::move(SerializedDeclsCollector.createPPCallbacks()));
+
----------------
unnecessary std::move
================
Comment at: clangd/ClangdUnit.cpp:325
+ std::move(ParsedDecls), std::move(ASTDiags),
+ std::move(SerializedDeclsCollector.getIRM()));
}
----------------
std::move(IRM)
================
Comment at: clangd/ClangdUnit.cpp:345
/// Finds declarations locations that a given source location refers to.
class DeclarationLocationsFinder : public index::IndexDataConsumer {
std::vector<Location> DeclarationLocations;
----------------
I think after rebase, all this code in DeclarationLocationsFinder will need to be moved around, probably inside clangd::findDefinitions?
================
Comment at: clangd/ClangdUnit.cpp:420
+
+ for (auto It = IncludeLocationMap.begin(); It != IncludeLocationMap.end();
+ ++It) {
----------------
```
for (auto &IncludeLoc : IncludeLocationMap) {
Range R = IncludeLoc.first;
if (isSameLine(R.start.line))
addLocation(URI::fromFile(IncludeLoc.second), R);
}
```
================
Comment at: clangd/ClangdUnit.cpp:427
+ }
+
// Also handle possible macro at the searched location.
----------------
Probably if an include location was found, we don't want to continue and it should early return.
================
Comment at: clangd/ClangdUnit.cpp:707
+ IncludeReferenceMap IRM;
+ CppFilePreambleCallbacks SerializedDeclsCollector(nullptr, IRM);
std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
----------------
I think you can put the construction of SerializedDeclsCollector back to where it was.
================
Comment at: clangd/ClangdUnit.cpp:776
+ std::move(CI), std::move(NewPreamble), std::move(ContentsBuffer),
+ PCHs, VFS, That->Logger, SerializedDeclsCollector.getIRM());
}
----------------
You can pass just IRM, so there is no need for the getIRM. We'll have eventually to transfer ownership to ParsedAST so maybe std::move(IRM) here?
================
Comment at: clangd/ClangdUnit.h:63
+class IncludeReferenceMap {
+ llvm::Optional<PathRef> findIncludeTargetAtLoc(Location Loc);
----------------
With the map being the only field left (see other comments), there is no need for a class I think. Maybe make a handy type alias for this?
```
using IncludeReferenceMap = std::unordered_map<Range, Path, RangeHash>;
```
================
Comment at: clangd/ClangdUnit.h:64
+class IncludeReferenceMap {
+ llvm::Optional<PathRef> findIncludeTargetAtLoc(Location Loc);
+
----------------
findIncludeTargetAtLoc is never used, remove
================
Comment at: clangd/ClangdUnit.h:68
+ std::unordered_map<Range, Path, RangeHash> IncludeLocationMap;
+ std::vector<std::pair<SourceRange, std::string>> DataVector;
+ std::vector<Range> RangeVector;
----------------
No need to keep this.
================
Comment at: clangd/ClangdUnit.h:69
+ std::vector<std::pair<SourceRange, std::string>> DataVector;
+ std::vector<Range> RangeVector;
+};
----------------
No need to keep this.
================
Comment at: clangd/ClangdUnit.h:103
+ IncludeReferenceMap takeIRM() { return IRM; };
+
----------------
We need to keep this copy of IRM alive for subsequent calls to "open definition" for the same ParsedAST. So it is correct to not do a std::move here. However, I think it should be changed to:
```
IncludeReferenceMap &getIRM()
```
and also the caller should assign to a reference.
================
Comment at: clangd/ClangdUnit.h:130
bool PreambleDeclsDeserialized;
+ std::vector<serialization::DeclID> PendingTopLevelDecls;
+ IncludeReferenceMap IRM;
----------------
this wrongly came back?
================
Comment at: unittests/clangd/ClangdTests.cpp:622
Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
- ASSERT_TRUE(!!Server.findDefinitions(FilePaths[FileIndex], Pos));
+ Server.findDefinitions(FilePaths[FileIndex], Pos);
};
----------------
why is this check removed?
================
Comment at: unittests/clangd/ClangdTests.cpp:751
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+ MockFSProvider FS;
----------------
Need to test includes outside preamble.
================
Comment at: unittests/clangd/ClangdTests.cpp:778
+
+ std::vector<Location> Locations = Server.findDefinitions(FooCpp, P).get().Value;
+ EXPECT_TRUE(!Locations.empty());
----------------
I get an "unchecked assertion" here before the Expected is not checked. I was able to replace it with:
```
auto ExpectedLocations = Server.findDefinitions(FooCpp, P);
ASSERT_TRUE(!!ExpectedLocations);
std::vector<Location> Locations = ExpectedLocations->Value;
```
================
Comment at: unittests/clangd/ClangdTests.cpp:784
+ check = check.substr(0, check.size() - 1);
+ ASSERT_EQ(check, FooH);
+ ASSERT_EQ(Locations[0].range.start.line, 0);
----------------
I get a test failure here.
```
../tools/clang/tools/extra/unittests/clangd/ClangdTests.cpp(785): Error: Expected: check
Which is: "clangd-test/foo."
To be equal to: FooH
Which is: { '/' (47, 0x2F), 'c' (99, 0x63), 'l' (108, 0x6C), 'a' (97, 0x61), 'n' (110, 0x6E), 'g' (103, 0x67), 'd' (100, 0x64), '-' (45, 0x2D), 't' (116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74), '/' (47, 0x2F), 'f' (102, 0x66), 'o' (111, 0x6F), 'o' (111, 0x6F), '.' (46, 0x2E), 'h' (104, 0x68) }
```
Not sure what's wrong. There is an additional front slash and missing ".h" at the end?
================
Comment at: unittests/clangd/ClangdTests.cpp:793
+
+ Locations = Server.findDefinitions(FooCpp, P3).get().Value;
+ EXPECT_TRUE(!Locations.empty());
----------------
Similar unchecked problem here, this works (although a bit yucky):
```
ExpectedLocations = Server.findDefinitions(FooCpp, P3);
ASSERT_TRUE(!!ExpectedLocations);
Locations = ExpectedLocations->Value;
EXPECT_TRUE(!Locations.empty());
// Test invalid include
Position P2 = Position{2, 11};
ExpectedLocations = Server.findDefinitions(FooCpp, P2);
ASSERT_TRUE(!!ExpectedLocations);
Locations = ExpectedLocations->Value;
EXPECT_TRUE(Locations.empty());
```
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
More information about the cfe-commits
mailing list