[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.


================
Comment at: clangd/ClangdUnit.cpp:81
 
+  std::map<SourceLocation, std::string> takeIncludeMap() {
+    return std::move(IncludeMap);
----------------
takeIncludeLocationMap?


================
Comment at: clangd/ClangdUnit.cpp:105
+    const SourceManager &SM = CI.getSourceManager();
+    unsigned n = SM.local_sloc_entry_size();
+    SmallVector<SourceLocation, 10> InclusionStack;
----------------
n -> NumSlocs?


================
Comment at: clangd/ClangdUnit.cpp:107
+    SmallVector<SourceLocation, 10> InclusionStack;
+    std::map<SourceLocation, std::string>::iterator it = IncludeMap.begin();
+
----------------
do you need that iterator?


================
Comment at: clangd/ClangdUnit.cpp:109
+
+    for (unsigned i = 0; i < n; ++i) {
+      bool Invalid = false;
----------------
i -> I


================
Comment at: clangd/ClangdUnit.cpp:137
+          FI.getContentCache()->OrigEntry->tryGetRealPathName();
+      if (FilePath.empty()) {
+        // FIXME: Does tryGetRealPathName return empty if and only if the path
----------------
I think you can just skip it if empty (continue)


================
Comment at: clangd/ClangdUnit.cpp:143
+      }
+      IncludeMap.insert(std::pair<SourceLocation, std::string>(
+          InclusionStack.front(), FilePath.str()));
----------------
I think you can do instead 
IncludeMap.insert({InclusionStack.front(), FilePath.str()});


================
Comment at: clangd/ClangdUnit.cpp:151
   std::vector<serialization::DeclID> TopLevelDeclIDs;
+  std::map<SourceLocation, std::string> IncludeMap;
 };
----------------
IncludeMap -> IncludeLocationMap ?


================
Comment at: clangd/ClangdUnit.cpp:800
 
-  void addDeclarationLocation(const SourceRange &ValSourceRange) {
+  bool isSameLine(unsigned line) const {
+    const SourceManager &SourceMgr = AST.getSourceManager();
----------------
line -> Line


================
Comment at: clangd/ClangdUnit.cpp:806
+  void addDeclarationLocation(const SourceRange &ValSourceRange,
+                              bool test = false) {
     const SourceManager &SourceMgr = AST.getSourceManager();
----------------
remove bool test?


================
Comment at: clangd/ClangdUnit.cpp:824
+
+  void addLocation(URI uri, Range R) {
+
----------------
uri -> Uri


================
Comment at: clangd/ClangdUnit.cpp:834
+
+    for (auto it = IncludeMap.begin(); it != IncludeMap.end(); ++it) {
+      SourceLocation L = it->first;
----------------
it -> It


================
Comment at: clangd/ClangdUnit.cpp:837
+      std::string &Path = it->second;
+      Range r = Range();
+      unsigned line = AST.getSourceManager().getSpellingLineNumber(L);
----------------
r -> R


================
Comment at: clangd/ClangdUnit.cpp:839
+      unsigned line = AST.getSourceManager().getSpellingLineNumber(L);
+      if (isSameLine(line))
+        addLocation(URI::fromFile(Path), Range());
----------------
line -> Line


================
Comment at: clangd/ClangdUnit.h:136
   std::vector<DiagWithFixIts> Diags;
+  std::map<SourceLocation, std::string> IncludeMap;
 };
----------------
IncludeLocationMap?


================
Comment at: clangd/ClangdUnit.h:267
+                                      clangd::Logger &Logger,
+                                      std::map<SourceLocation, std::string>);
 
----------------
do you want to add a name to the parameter here?


================
Comment at: unittests/clangd/ClangdTests.cpp:902
 
-TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
   MockFSProvider FS;
----------------
the test "CheckSourceHeaderSwitch" was removed


https://reviews.llvm.org/D38639