[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include

2019-08-27 Thread Sam McCall via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL370029: [clangd] Fix toHalfOpenFileRange where start/end 
endpoints are in different… (authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66590?vs=216609=217325#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66590/new/

https://reviews.llvm.org/D66590

Files:
  clang-tools-extra/trunk/clangd/SourceCode.cpp
  clang-tools-extra/trunk/clangd/SourceCode.h
  clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp

Index: clang-tools-extra/trunk/clangd/SourceCode.h
===
--- clang-tools-extra/trunk/clangd/SourceCode.h
+++ clang-tools-extra/trunk/clangd/SourceCode.h
@@ -83,6 +83,11 @@
 /// the main file.
 bool isInsideMainFile(SourceLocation Loc, const SourceManager );
 
+/// Returns the #include location through which IncludedFIle was loaded.
+/// Where SM.getIncludeLoc() returns the location of the *filename*, which may
+/// be in a macro, includeHashLoc() returns the location of the #.
+SourceLocation includeHashLoc(FileID IncludedFile, const SourceManager );
+
 /// Returns true if the token at Loc is spelled in the source code.
 /// This is not the case for:
 ///   * symbols formed via macro concatenation, the spelling location will
Index: clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
@@ -11,9 +11,11 @@
 #include "SourceCode.h"
 #include "TestTU.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Format/Format.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Annotations.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -505,6 +507,53 @@
   CheckRange("f");
 }
 
+TEST(SourceCodeTests, HalfOpenFileRangePathologicalPreprocessor) {
+  const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+[[#include "Expand.inc"
+br^eak]];
+}
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+  auto AST = TU.build();
+
+  const auto  = cast(findDecl(AST, "test"));
+  const auto  = cast(Func.getBody());
+  const auto  = cast(*Body->child_begin());
+  llvm::Optional Range = toHalfOpenFileRange(
+  AST.getSourceManager(), AST.getASTContext().getLangOpts(),
+  Loop->getSourceRange());
+  ASSERT_TRUE(Range) << "Failed to get file range";
+  EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getBegin()),
+Test.llvm::Annotations::range().Begin);
+  EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getEnd()),
+Test.llvm::Annotations::range().End);
+}
+
+TEST(SourceCodeTests, IncludeHashLoc) {
+  const char *Case = R"cpp(
+$foo^#include "foo.inc"
+#define HEADER "bar.inc"
+  $bar^#  include HEADER
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["foo.inc"] = "int foo;\n";
+  TU.AdditionalFiles["bar.inc"] = "int bar;\n";
+  auto AST = TU.build();
+  const auto& SM = AST.getSourceManager();
+
+  FileID Foo = SM.getFileID(findDecl(AST, "foo").getLocation());
+  EXPECT_EQ(SM.getFileOffset(includeHashLoc(Foo, SM)),
+Test.llvm::Annotations::point("foo"));
+  FileID Bar = SM.getFileID(findDecl(AST, "bar").getLocation());
+  EXPECT_EQ(SM.getFileOffset(includeHashLoc(Bar, SM)),
+Test.llvm::Annotations::point("foo"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
@@ -346,6 +346,25 @@
   }
 }
 
+TEST(SelectionTest, PathologicalPreprocessor) {
+  const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+#include "Expand.inc"
+br^eak;
+}
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty());
+  auto T = makeSelectionTree(Case, AST);
+
+  EXPECT_EQ("BreakStmt", T.commonAncestor()->kind());
+  EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
+}
+
 TEST(SelectionTest, Implicit) {
   const char* Test = R"cpp(
 struct S { S(const char*); };
Index: clang-tools-extra/trunk/clangd/SourceCode.cpp

[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include

2019-08-27 Thread Sam McCall via Phabricator via cfe-commits
sammccall marked an inline comment as done.
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:284
+  return SM.getComposedLoc(IncludingFile, Offset);
+if (Buf[Offset] == '\n' || Offset == 0) // no hash, what's going on?
+  return SourceLocation();

SureYeaah wrote:
> nit: use this as a while condition?
> 
> ```
> while(Offset-- && Buf[Offset] != '\n')
> ```
Ooh, clever... a little too subtle for my taste though :-)
(In particular, allowing the variable to overflow but then ensuring we never 
read from it...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66590/new/

https://reviews.llvm.org/D66590



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include

2019-08-23 Thread Shaurya Gupta via Phabricator via cfe-commits
SureYeaah accepted this revision.
SureYeaah added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang-tools-extra/clangd/SourceCode.cpp:284
+  return SM.getComposedLoc(IncludingFile, Offset);
+if (Buf[Offset] == '\n' || Offset == 0) // no hash, what's going on?
+  return SourceLocation();

nit: use this as a while condition?

```
while(Offset-- && Buf[Offset] != '\n')
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66590/new/

https://reviews.llvm.org/D66590



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include

2019-08-22 Thread Sam McCall via Phabricator via cfe-commits
sammccall marked 4 inline comments as done.
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:318
-// Check if two locations have the same file id.
-static bool inSameFile(SourceLocation Loc1, SourceLocation Loc2,
-   const SourceManager ) {

I dropped this function because it's just SM.isSpelledInSameFile



Comment at: clang-tools-extra/clangd/SourceCode.cpp:338
+  : R2.getBegin();
+  SourceLocation End =
+  SM.isBeforeInTranslationUnit(getLocForTokenEnd(R1.getEnd(), SM, 
LangOpts),

this is an unrelated bug fix: E1 < E2, std::min() etc don't do anything 
sensible for SourceLocation



Comment at: clang-tools-extra/clangd/SourceCode.cpp:358
Begin = Begin.isFileID()
-   ? SourceLocation()
+   ? includeHashLoc(SM.getFileID(Begin), SM)
: SM.getImmediateExpansionRange(Begin).getBegin()) {

This is half of the real fix: once we've hit the top of the macro tree, we 
should walk up the `#include` stack in case we find a common ancestor there.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:443
+  SourceRange Result =
+  rangeInCommonFile(unionTokenRange(R1, R2, SM, LangOpts), SM, LangOpts);
   unsigned TokLen = getTokenLengthAtLoc(Result.getEnd(), SM, LangOpts);

this is one part of the main fix: the start and endpoint may be in different 
real files in the presence of #include


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66590/new/

https://reviews.llvm.org/D66590



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include

2019-08-22 Thread Sam McCall via Phabricator via cfe-commits
sammccall updated this revision to Diff 216609.
sammccall added a comment.

kick phabricator to try to get it to mail


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66590/new/

https://reviews.llvm.org/D66590

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/unittests/SelectionTests.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
@@ -11,9 +11,11 @@
 #include "SourceCode.h"
 #include "TestTU.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Format/Format.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Annotations.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -505,6 +507,53 @@
   CheckRange("f");
 }
 
+TEST(SourceCodeTests, HalfOpenFileRangePathologicalPreprocessor) {
+  const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+[[#include "Expand.inc"
+br^eak]];
+}
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+  auto AST = TU.build();
+
+  const auto  = cast(findDecl(AST, "test"));
+  const auto  = cast(Func.getBody());
+  const auto  = cast(*Body->child_begin());
+  llvm::Optional Range = toHalfOpenFileRange(
+  AST.getSourceManager(), AST.getASTContext().getLangOpts(),
+  Loop->getSourceRange());
+  ASSERT_TRUE(Range) << "Failed to get file range";
+  EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getBegin()),
+Test.llvm::Annotations::range().Begin);
+  EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getEnd()),
+Test.llvm::Annotations::range().End);
+}
+
+TEST(SourceCodeTests, IncludeHashLoc) {
+  const char *Case = R"cpp(
+$foo^#include "foo.inc"
+#define HEADER "bar.inc"
+  $bar^#  include HEADER
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["foo.inc"] = "int foo;\n";
+  TU.AdditionalFiles["bar.inc"] = "int bar;\n";
+  auto AST = TU.build();
+  const auto& SM = AST.getSourceManager();
+
+  FileID Foo = SM.getFileID(findDecl(AST, "foo").getLocation());
+  EXPECT_EQ(SM.getFileOffset(includeHashLoc(Foo, SM)),
+Test.llvm::Annotations::point("foo"));
+  FileID Bar = SM.getFileID(findDecl(AST, "bar").getLocation());
+  EXPECT_EQ(SM.getFileOffset(includeHashLoc(Bar, SM)),
+Test.llvm::Annotations::point("foo"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -346,6 +346,25 @@
   }
 }
 
+TEST(SelectionTest, PathologicalPreprocessor) {
+  const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+#include "Expand.inc"
+br^eak;
+}
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty());
+  auto T = makeSelectionTree(Case, AST);
+
+  EXPECT_EQ("BreakStmt", T.commonAncestor()->kind());
+  EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
+}
+
 TEST(SelectionTest, Implicit) {
   const char* Test = R"cpp(
 struct S { S(const char*); };
Index: clang-tools-extra/clangd/SourceCode.h
===
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -83,6 +83,11 @@
 /// the main file.
 bool isInsideMainFile(SourceLocation Loc, const SourceManager );
 
+/// Returns the #include location through which IncludedFIle was loaded.
+/// Where SM.getIncludeLoc() returns the location of the *filename*, which may
+/// be in a macro, includeHashLoc() returns the location of the #.
+SourceLocation includeHashLoc(FileID IncludedFile, const SourceManager );
+
 /// Returns true if the token at Loc is spelled in the source code.
 /// This is not the case for:
 ///   * symbols formed via macro concatenation, the spelling location will
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -264,6 +264,29 @@
   return L == R.getEnd() || halfOpenRangeContains(Mgr, R, L);
 }
 
+SourceLocation includeHashLoc(FileID IncludedFile, const SourceManager ) {
+  

[PATCH] D66590: [clangd] Fix toHalfOpenFileRange where start/end endpoints are in different files due to #include

2019-08-22 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: SureYeaah.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

https://github.com/clangd/clangd/issues/129


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66590

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/unittests/SelectionTests.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
@@ -11,9 +11,11 @@
 #include "SourceCode.h"
 #include "TestTU.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Format/Format.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Annotations.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -505,6 +507,53 @@
   CheckRange("f");
 }
 
+TEST(SourceCodeTests, HalfOpenFileRangePathologicalPreprocessor) {
+  const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+[[#include "Expand.inc"
+br^eak]];
+}
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+  auto AST = TU.build();
+
+  const auto  = cast(findDecl(AST, "test"));
+  const auto  = cast(Func.getBody());
+  const auto  = cast(*Body->child_begin());
+  llvm::Optional Range = toHalfOpenFileRange(
+  AST.getSourceManager(), AST.getASTContext().getLangOpts(),
+  Loop->getSourceRange());
+  ASSERT_TRUE(Range) << "Failed to get file range";
+  EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getBegin()),
+Test.llvm::Annotations::range().Begin);
+  EXPECT_EQ(AST.getSourceManager().getFileOffset(Range->getEnd()),
+Test.llvm::Annotations::range().End);
+}
+
+TEST(SourceCodeTests, IncludeHashLoc) {
+  const char *Case = R"cpp(
+$foo^#include "foo.inc"
+#define HEADER "bar.inc"
+  $bar^#  include HEADER
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["foo.inc"] = "int foo;\n";
+  TU.AdditionalFiles["bar.inc"] = "int bar;\n";
+  auto AST = TU.build();
+  const auto& SM = AST.getSourceManager();
+
+  FileID Foo = SM.getFileID(findDecl(AST, "foo").getLocation());
+  EXPECT_EQ(SM.getFileOffset(includeHashLoc(Foo, SM)),
+Test.llvm::Annotations::point("foo"));
+  FileID Bar = SM.getFileID(findDecl(AST, "bar").getLocation());
+  EXPECT_EQ(SM.getFileOffset(includeHashLoc(Bar, SM)),
+Test.llvm::Annotations::point("foo"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -346,6 +346,25 @@
   }
 }
 
+TEST(SelectionTest, PathologicalPreprocessor) {
+  const char *Case = R"cpp(
+#define MACRO while(1)
+void test() {
+#include "Expand.inc"
+br^eak;
+}
+  )cpp";
+  Annotations Test(Case);
+  auto TU = TestTU::withCode(Test.code());
+  TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty());
+  auto T = makeSelectionTree(Case, AST);
+
+  EXPECT_EQ("BreakStmt", T.commonAncestor()->kind());
+  EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
+}
+
 TEST(SelectionTest, Implicit) {
   const char* Test = R"cpp(
 struct S { S(const char*); };
Index: clang-tools-extra/clangd/SourceCode.h
===
--- clang-tools-extra/clangd/SourceCode.h
+++ clang-tools-extra/clangd/SourceCode.h
@@ -83,6 +83,11 @@
 /// the main file.
 bool isInsideMainFile(SourceLocation Loc, const SourceManager );
 
+/// Returns the #include location through which IncludedFIle was loaded.
+/// Where SM.getIncludeLoc() returns the location of the *filename*, which may
+/// be in a macro, includeHashLoc() returns the location of the #.
+SourceLocation includeHashLoc(FileID IncludedFile, const SourceManager );
+
 /// Returns true if the token at Loc is spelled in the source code.
 /// This is not the case for:
 ///   * symbols formed via macro concatenation, the spelling location will
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -264,6 +264,29 @@
   return L == R.getEnd() || halfOpenRangeContains(Mgr, R, L);
 }
 
+SourceLocation includeHashLoc(FileID IncludedFile, const