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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:107
 
+class CollectMainFileMacroExpansions : public PPCallbacks {
+  const SourceManager &SM;
----------------
Maybe make this part of `CollectMainFileMacros`? It looks like a natural fit there and we won't need another instance of `PPCallbacks`.


================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:120
+    SourceLocation L = MacroNameTok.getLocation();
+    if (isInsideMainFile(SM.getSpellingLoc(L), SM) && !L.isMacroID())
+      MainFileMacroLocs.push_back(L);
----------------
I believe `getSpellingLoc` is a no-op if `isMacroID()` is false. Maybe simplify to:
```
if (!L.isMacroID() && isInsideMainFile(L, SM))
  ...
```


================
Comment at: clang-tools-extra/clangd/ClangdUnit.h:119
 
+  const std::vector<SourceLocation> &getMainFileMacroExpansionLocations() const;
   /// Tokens recorded while parsing the main file.
----------------
NIT: return `llvm::ArrayRef<SourceLocation>` instead of `const vector`


================
Comment at: clang-tools-extra/clangd/ClangdUnit.h:119
 
+  const std::vector<SourceLocation> &getMainFileMacroExpansionLocations() const;
   /// Tokens recorded while parsing the main file.
----------------
ilya-biryukov wrote:
> NIT: return `llvm::ArrayRef<SourceLocation>` instead of `const vector`
NIT: maybe shorten the name to `getMainFileExpansions`?


================
Comment at: clang-tools-extra/clangd/ClangdUnit.h:150
+  /// Does not include expansions from inside other macro expansions.
+  std::vector<SourceLocation> MainFileMacroExpLocs;
   // Data, stored after parsing.
----------------
NIT: a comment like this or a similar one is probably also useful in the public accessor.


================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:248
+TEST(ClangdUnitTest, CollectsMainFileMacroExpansions) {
+  Annotations TestCase(R"cpp(
+    #define MACRO_ARGS(X, Y) X Y
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > 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
> > ```
> Does clangd handle `.inc` files differently from `.h`? Because if it doesn't shouldn't ` case 1 cover case 4 as well ?
Right, sorry, was rushing to finish a comment. `.inc` and `.h` are handled exactly the same.
There is only a difference if they are in the preamble or not.


================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:260
+    // Macros from token concatenations included.
+    #define CONCAT(X) X##1()
+    #define MACRO1() 123
----------------
Could we also test?
```
#define PREPEND(X) Foo##X
```

It'll probably be the same, but still interesting to see whether it's any differnet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66928