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