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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
