[PATCH] D66928: [clangd] Collecting main file macro expansion locations in ParsedAST.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Looks great, just requests for comments and more test-cases from my side



================
Comment at: clang-tools-extra/clangd/ClangdUnit.h:148
 
+  /// All macro expansion locations in the main file.
+  std::vector<SourceLocation> MainFileMacroExpLocs;
----------------
NIT: clarify that this is the start location of the identifier that was macro-expanded.
"expansion location" is an overloaded term


================
Comment at: clang-tools-extra/clangd/ClangdUnit.h:149
+  /// All macro expansion locations in the main file.
+  std::vector<SourceLocation> MainFileMacroExpLocs;
   // Data, stored after parsing.
----------------
Could you please document whether these include:
1. locations outside the main file
2. locations from inside other macro expansions, i.e. those that have `isMacroID() == true`.


================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:248
+TEST(ClangdUnitTest, CollectsMainFileMacroExpansions) {
+  Annotations TestCase(R"cpp(
+    #define MACRO_ARGS(X, Y) X Y
----------------
Could you add a few more interesting cases?

1. Macros outside the main file **and** the preamble:
```
// foo.inc
int a = ID(1);

// foo.cpp
#define ID(X) X

int b;
#include "foo.inc"
```

2. macro expansions from token concatenations
```
#define FOO(X) X##1()
#define MACRO1() 123

int a = FOO(MACRO);
```
3. Macro names inside other macros:
```
#define FOO BAR
#define BAR 1


int a = FOO; // should BAR at line 1 be highlighted?

```

4. #include not part of the preamble:
```
#define FOO 1

// Preamble ends here.
int a = 10;
#include "some_file_with_macros.h" // <-- should not get any macros from here
```


================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:264
+  for (int I = 0, End = MacroExpansionLocations.size(); I < End; ++I)
+    MacroExpansionPositions[I] =
+        sourceLocToPosition(AST.getSourceManager(), MacroExpansionLocations[I]);
----------------
NIT: maybe use `push_back` and for-each loop, the resulting code should be simpler and readability is more important for the test code than performance:
```
std::vector<Position> Positions;
for (SourceLocation Loc : AST.getMainFileMacros())
  Positions.push_back(sourceLocToPosition(Loc));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66928