[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG538c2753f3ec: [clangd] locateMacroAt handles patched macros 
(authored by kadircet).

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/Preamble.h
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -9,14 +9,19 @@
 #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 "XRefs.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
@@ -28,6 +33,7 @@
 
 using testing::Contains;
 using testing::Field;
+using testing::Matcher;
 using testing::MatchesRegex;
 
 namespace clang {
@@ -199,7 +205,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);
 }
 
@@ -228,7 +234,8 @@
 #define BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -238,7 +245,8 @@
 
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -248,7 +256,8 @@
 BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 3
+#define BAR
 )cpp",
   },
   };
@@ -275,8 +284,10 @@
   )cpp");
 
   llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
-#define BAR\(X, Y\) X Y
-#define BAR\(X\) X
+#line 2
+#define BAR\(X, Y\) X Y
+#line 3
+#define BAR\(X\) X
 )cpp");
   EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
   MatchesRegex(ExpectedPatch.str()));
@@ -286,6 +297,193 @@
   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());
+ASSERT_TRUE(FoundMacro);
+EXPECT_THAT(FoundMacro->Name, "FOO");
+
+auto MacroLoc = FoundMacro->NameLoc;
+EXPECT_EQ(SM.getFileID(MacroLoc), SM.getMainFileID());
+EXPECT_EQ(SM.getFileOffset(MacroLoc), Modified.point("def"));
+  }
+}
+
+TEST(PreamblePatchTest, LocateMacroAtDeletion) {
+  {
+//

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 266639.
kadircet added a comment.

- Rebase


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/Preamble.h
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -9,14 +9,19 @@
 #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 "XRefs.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
@@ -28,6 +33,7 @@
 
 using testing::Contains;
 using testing::Field;
+using testing::Matcher;
 using testing::MatchesRegex;
 
 namespace clang {
@@ -199,7 +205,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);
 }
 
@@ -228,7 +234,8 @@
 #define BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -238,7 +245,8 @@
 
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -248,7 +256,8 @@
 BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 3
+#define BAR
 )cpp",
   },
   };
@@ -275,8 +284,10 @@
   )cpp");
 
   llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
-#define BAR\(X, Y\) X Y
-#define BAR\(X\) X
+#line 2
+#define BAR\(X, Y\) X Y
+#line 3
+#define BAR\(X\) X
 )cpp");
   EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
   MatchesRegex(ExpectedPatch.str()));
@@ -286,6 +297,193 @@
   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());
+ASSERT_TRUE(FoundMacro);
+EXPECT_THAT(FoundMacro->Name, "FOO");
+
+auto MacroLoc = FoundMacro->NameLoc;
+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 

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 266187.
kadircet added a comment.

- Rebase


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/Preamble.h
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -9,14 +9,19 @@
 #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 "XRefs.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
@@ -28,6 +33,7 @@
 
 using testing::Contains;
 using testing::Field;
+using testing::Matcher;
 using testing::MatchesRegex;
 
 namespace clang {
@@ -199,7 +205,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);
 }
 
@@ -228,7 +234,8 @@
 #define BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -238,7 +245,8 @@
 
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -248,7 +256,8 @@
 BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 3
+#define BAR
 )cpp",
   },
   };
@@ -275,8 +284,10 @@
   )cpp");
 
   llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
-#define BAR\(X, Y\) X Y
-#define BAR\(X\) X
+#line 2
+#define BAR\(X, Y\) X Y
+#line 3
+#define BAR\(X\) X
 )cpp");
   EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
   MatchesRegex(ExpectedPatch.str()));
@@ -286,6 +297,193 @@
   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());
+ASSERT_TRUE(FoundMacro);
+EXPECT_THAT(FoundMacro->Name, "FOO");
+
+auto MacroLoc = FoundMacro->NameLoc;
+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 

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 266128.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Add tests for preamble-patch translation logic


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/Preamble.h
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -9,14 +9,19 @@
 #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 "XRefs.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
@@ -28,6 +33,7 @@
 
 using testing::Contains;
 using testing::Field;
+using testing::Matcher;
 using testing::MatchesRegex;
 
 namespace clang {
@@ -199,7 +205,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);
 }
 
@@ -228,7 +234,8 @@
 #define BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -238,7 +245,8 @@
 
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -248,7 +256,8 @@
 BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 3
+#define BAR
 )cpp",
   },
   };
@@ -275,8 +284,10 @@
   )cpp");
 
   llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
-#define BAR\(X, Y\) X Y
-#define BAR\(X\) X
+#line 2
+#define BAR\(X, Y\) X Y
+#line 3
+#define BAR\(X\) X
 )cpp");
   EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
   MatchesRegex(ExpectedPatch.str()));
@@ -286,6 +297,193 @@
   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());
+ASSERT_TRUE(FoundMacro);
+EXPECT_THAT(FoundMacro->Name, "FOO");
+
+auto MacroLoc = FoundMacro->NameLoc;
+EXPECT_EQ(SM.getFileID(MacroLoc), SM.getMainFileID());
+EXPECT_EQ(SM.getFileOffset(MacroLoc), Modified.point("def"));
+  }
+}
+
+TEST(PreamblePatchTest, LocateMacroAtDeletion) {
+  {
+// We don't p

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Looks great, ship it!




Comment at: clang-tools-extra/clangd/Preamble.cpp:154
+OS << "\\\n" << std::string(TargetColumn, ' ');
+// Decrement because returned string has a line break before directive now.
+--DirectiveLine;

nit: not "before the directive", just before the DirectiveRange.begin()



Comment at: clang-tools-extra/clangd/Preamble.cpp:157
+  } else {
+// There is enough space for Prefix and space before directive, use it.
+// We try to squeeze the Prefix into the same line whenever we can, as

nit: I think it'd be easier to understand the other case if this one came 
first, as it's the fallback



Comment at: clang-tools-extra/clangd/Preamble.h:136
+/// using presumed locations. Returns \p Loc if it isn't inside preamble patch.
+SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
+  const SourceManager &SM);

Can we have a unit-test for this function?
I think the minimal thing (least entangled with PreamblePatch construction) is 
to manually write a snippet that looks like a preamble patch like
```
# line 3 main.cpp
int foo();
```
 map it into a TestTU with AdditionalFiles, set `ExtraArgs = 
{"-include","patch.h"}` and then get the location with `findDecl`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80198



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

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:133
+  auto DecompLoc = SM.getDecomposedLoc(DirectiveRange.getBegin());
+  DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+  auto SpaceBefore = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1;

sammccall wrote:
> This is only going to give you something useful for file IDs, not macro 
> expansions. Whose responsibility is it to make sure there's no macro IDs here?
For `define` directives this is already true as `MacroInfo`'s definition range 
is a file range(as macro bodies are lexed without macro expansions).

For any other directive with macro expansions, we only care about the expansion 
locations as we want to spell out the directive as written. So we can just map 
back to expansion locs here.



Comment at: clang-tools-extra/clangd/Preamble.cpp:136
+
+  // Pad with spaces before DirectiveRange to make sure it will be on right
+  // column when patched.

sammccall wrote:
> again this comment only applyes to one branch
why ? we pad in both cases, starting from the beginning of the line in first 
branch and starting after Prefix in the second.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:975
+
+  // Macro definitions could be injected through preamble patch. These contain
+  // line directives to hint their original location in main file.

sammccall wrote:
> OK, this is fairly horrible...
> 
> I want to say the preamble patch location translation should be a separate 
> function rather than coupled to macro-specific stuff.
> (Then we don't even need the extra struct member, but we can still keep it to 
> "remind" people the translation is needed, if we like).
> 
> But of course we didn't preserve the formatting in the preamble patch, so 
> `getPresumedLoc()` doesn't give us the right location, it just gives us 
> something on the right line that we then need to re-parse...
> 
> This really isn't looking like a great tradeoff to me anymore (and I know I 
> suggested it).
> How much work do you think it is (compared to say, the logic here plus doing 
> it in ~2 more places) to give the preamblepatch the invariant that the 
> PresumedLoc has a usable col as well.
> The original implementation did padding with spaces, but I guess we could as 
> well have the preamble patching stuff splice the actual source code...
as discussed offline, changed patching logic to put directive in correct column 
in addition to line. Now we can just make use of presumedloc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80198



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

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 266111.
kadircet marked 10 inline comments as done.
kadircet added a comment.

- Extract preamble-patch location translation to a helper:
  - Migrate usage in include collector
  - Make preamble patch header name part of the check.
- Handle macro ids in spellDirective
- Tweak comments/names


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/Preamble.h
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -9,14 +9,19 @@
 #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 "XRefs.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
@@ -28,6 +33,7 @@
 
 using testing::Contains;
 using testing::Field;
+using testing::Matcher;
 using testing::MatchesRegex;
 
 namespace clang {
@@ -199,7 +205,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);
 }
 
@@ -228,7 +234,8 @@
 #define BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -238,7 +245,8 @@
 
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -248,7 +256,8 @@
 BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 3
+#define BAR
 )cpp",
   },
   };
@@ -275,8 +284,10 @@
   )cpp");
 
   llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
-#define BAR\(X, Y\) X Y
-#define BAR\(X\) X
+#line 2
+#define BAR\(X, Y\) X Y
+#line 3
+#define BAR\(X\) X
 )cpp");
   EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
   MatchesRegex(ExpectedPatch.str()));
@@ -286,6 +297,169 @@
   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());
+ASSERT_TRUE(FoundMacro);
+EXPECT_THAT(FoundMacro->Name, "FOO");
+
+auto MacroLoc = FoundMacro->NameLoc;
+EXPECT_EQ(SM.getFileID(MacroLoc), SM.get

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(BTW this looks miles better to me, thanks for persistence!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80198



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

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:123
 
+// Spells directive in \p DirectiveRange while prepending it with \p Prefix.
+// Returned string can be used with a #line directive on line \p DirectiveLine

> Spells directive in \p DirectiveRange while prepending it with \p Prefix

I wouldn't understand what this meant if I didn't already know :-) Maybe:

```
// Formats a PP directive consisting of Prefix (e.g. "#define ") and Body ("X 
10").
// The formatting is copied so that the tokens in Body have PresumedLocs with
// correct columns and lines.
```



Comment at: clang-tools-extra/clangd/Preamble.cpp:127
+std::string spellDirective(llvm::StringRef Prefix, SourceRange DirectiveRange,
+   unsigned &DirectiveLine, const SourceManager &SM) {
+  std::string SpelledDirective;

nit: move out-param to end?



Comment at: clang-tools-extra/clangd/Preamble.cpp:133
+  auto DecompLoc = SM.getDecomposedLoc(DirectiveRange.getBegin());
+  DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+  auto SpaceBefore = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1;

This is only going to give you something useful for file IDs, not macro 
expansions. Whose responsibility is it to make sure there's no macro IDs here?



Comment at: clang-tools-extra/clangd/Preamble.cpp:134
+  DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+  auto SpaceBefore = SM.getColumnNumber(DecompLoc.first, DecompLoc.second) - 1;
+

this name seems a bit confusing to me, since it only covers one case.
TargetColumn?
`if (TargetColumn < Prefix.size())` reads a lot celarer I think.



Comment at: clang-tools-extra/clangd/Preamble.cpp:136
+
+  // Pad with spaces before DirectiveRange to make sure it will be on right
+  // column when patched.

again this comment only applyes to one branch



Comment at: clang-tools-extra/clangd/Preamble.cpp:139
+  if (SpaceBefore < Prefix.size()) {
+// Prefix was longer than the space we had. Pad from start of a new line.
+OS << "\\\n" << std::string(SpaceBefore, ' ');

Rather than "pad from the start of a new line" it might be clearer to give an 
example here:
```
... We produce e.g.:
#line N-1
#define \
   X 10
```



Comment at: clang-tools-extra/clangd/Preamble.cpp:149
+  }
+  OS << toSourceCode(SM, DirectiveRange);
+  return OS.str();

this asserts isValidFileRange, so we need this to be true :-)



Comment at: clang-tools-extra/clangd/SourceCode.cpp:976
+  // #line directives to hint their original location in main file.
+  auto DefFile = SM.getFileID(NameLoc);
+  auto IncludeLoc = SM.getIncludeLoc(DefFile);

I think this block can now be lifted to a function in SourceCode or Preamble or 
so?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80198



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

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 266047.
kadircet marked 3 inline comments as done.
kadircet added a comment.

Instead of re-lexing at use time, put macro identifier on correct column while
patching and make use of presumed locations at use time.


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,14 +9,19 @@
 #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 "XRefs.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
@@ -28,6 +33,7 @@
 
 using testing::Contains;
 using testing::Field;
+using testing::Matcher;
 using testing::MatchesRegex;
 
 namespace clang {
@@ -199,7 +205,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);
 }
 
@@ -228,7 +234,8 @@
 #define BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -238,7 +245,8 @@
 
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 2
+#define BAR
 )cpp",
   },
   // multiline macro
@@ -248,7 +256,8 @@
 BAR
 [[BAR]])cpp",
   R"cpp(#line 0 ".*main.cpp"
-#define BAR
+#line 3
+#define BAR
 )cpp",
   },
   };
@@ -275,8 +284,10 @@
   )cpp");
 
   llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
-#define BAR\(X, Y\) X Y
-#define BAR\(X\) X
+#line 2
+#define BAR\(X, Y\) X Y
+#line 3
+#define BAR\(X\) X
 )cpp");
   EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
   MatchesRegex(ExpectedPatch.str()));
@@ -286,6 +297,171 @@
   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
+  {
+  "",

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:975
+
+  // Macro definitions could be injected through preamble patch. These contain
+  // line directives to hint their original location in main file.

OK, this is fairly horrible...

I want to say the preamble patch location translation should be a separate 
function rather than coupled to macro-specific stuff.
(Then we don't even need the extra struct member, but we can still keep it to 
"remind" people the translation is needed, if we like).

But of course we didn't preserve the formatting in the preamble patch, so 
`getPresumedLoc()` doesn't give us the right location, it just gives us 
something on the right line that we then need to re-parse...

This really isn't looking like a great tradeoff to me anymore (and I know I 
suggested it).
How much work do you think it is (compared to say, the logic here plus doing it 
in ~2 more places) to give the preamblepatch the invariant that the PresumedLoc 
has a usable col as well.
The original implementation did padding with spaces, but I guess we could as 
well have the preamble patching stuff splice the actual source code...



Comment at: clang-tools-extra/clangd/SourceCode.h:295
   const MacroInfo *Info;
+  /// Subject to #line directive substitution. E.g. a macro defined in a
+  /// built-in file might have an identifier location inside the main file.

It's not obvious to me that this comment describes a special case but the field 
is valid in general.
Maybe
```
/// Location of the identifier that names the macro.
/// Unlike Info->Location, this translates preamble-patch locations to 
main-file locations.
```



Comment at: clang-tools-extra/clangd/SourceCode.h:297
+  /// built-in file might have an identifier location inside the main file.
+  SourceLocation IdentLoc;
 };

nit: call this NameLoc or NameLocation?
ident is *slightly* vague


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80198



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

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 265195.
kadircet added a comment.

- Rebase


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,14 +9,19 @@
 #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 "XRefs.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
@@ -28,6 +33,7 @@
 
 using testing::Contains;
 using testing::Field;
+using testing::Matcher;
 
 namespace clang {
 namespace clangd {
@@ -198,7 +204,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 +249,171 @@
   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(Macro

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

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 &S

[PATCH] D80198: [clangd] locateMacroAt handles patched macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Depends on D79992 .


Repository:
  rG LLVM Github Monorepo

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
@@ -198,7 +198,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 +243,101 @@
   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->DefRange.getBegin();
+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);
+