This patch adds the cross references for Macros in the MainFile.
We add references for the main file to the ParsedAST. We query the
references from it using the SymbolID.
Xref outside main file will be added to the index in a separate patch.
Details
- Reviewers
hokein
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clangd/CollectMacros.h | ||
---|---|---|
29 | I think the Ranges and MacrosRefs have a lot of duplications, it is wasteful to store a same range twice. We don't need the MainFilePath, the callers should know the corresponding file path for the AST. Something like llvm::DenseMap<SymbolID, std::vector<Range>> Refs; should be enough. symbol id for macro generation is required the definition location, but only defined macros have it, we may need an extra vector to store ranges for undefined macros. #ifndef abc // there is no macro info* for abc, so getSymbolID will fail, but we still want it, e.g. in semantic highlighting. #endif There may be more tricky cases, it would be nice to have in the test. #define abc 1 #undef abc #define abc 2 #undef abc | |
clang-tools-extra/clangd/XRefs.cpp | ||
900 | this is not completed, it only shows the macro refs in main file, we still need to query the index to get the rest (but our index doesn't collect macro at the moment). I'd prefer not doing this in this patch until our index is ready. For testing, I think we can create a new test file for CollectMainFileMacros. |
clang-tools-extra/clangd/XRefs.cpp | ||
---|---|---|
900 | Yup. This patch is just for the main file. The index and the lookup will be modified in the next patch. Wanted to keep this small. |
Build result: pass - 59967 tests passed, 0 failed and 763 were skipped.
Log files: console-log.txt, CMakeCache.txt
clang-tools-extra/clangd/CollectMacros.h | ||
---|---|---|
29 |
|
Build result: pass - 59989 tests passed, 0 failed and 763 were skipped.
Log files: console-log.txt, CMakeCache.txt
Build result: pass - 59989 tests passed, 0 failed and 763 were skipped.
Log files: console-log.txt, CMakeCache.txt
clang-tools-extra/clangd/CollectMacros.h | ||
---|---|---|
29 |
will we encounter duplications? My theory is that we should not. I think it is fine to use vector. And you also use vector for the UnknownMacros below. | |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
313–314 | nit: the document was stale, expansions => references. | |
clang-tools-extra/clangd/XRefs.cpp | ||
900 | this would enable the xref for macros, but the feature is not ready yet. I think we'd better add this code when everything is ready. | |
clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp | ||
57 | could you add test case where there is an undefined macro reference before the macro definition, like #ifndef abc #define abc #endif #endif I think the first abc is unknown, the second abc is defined? | |
81 | I might be missing something, I didn't get the motivation of using numbers in the annotation, the code seems just collecting all annotated ranges regardless the value of the number and compare to the actual ranges. it doesn't verify that e.g. in your 2rd cases, the two macros abc are different symbols. How about just verifying a single defined macro for each case? e.g. the below case, we try to get the symbol id (call locatedMacroAt) from the T.point(), retrieve the responding ranges from the MainFileMacros, compare to the annotated ranges. #define [[F^oo]] 1 int abc = [[Foo]]; #undef [[Foo]] |
clang-tools-extra/clangd/XRefs.cpp | ||
---|---|---|
900 | Removed looking up xref. | |
clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp | ||
81 | Since the output of CollectMacros is a mapping from SymbolID -> vector, I wanted to verify that we form correct groups of ranges (grouped by SymbolID).
We actually group the ranges by their annotation and form Matcher <std::vector<Matcher <std::vector<Range>>>>. So it checks for the correct grouping too. |
looks like you forgot to upload the latest patch.
clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp | ||
---|---|---|
81 | I see, having a complex Matcher <std::vector<Matcher <std::vector<Range>>>> would hurt code readability, and the error message is also not friendly when there are failed testcases. How about extending it like below? To improve the error message, I think we should do a string comparison: the test annotation v.s the raw code with the actual macro references annotated (we'd need to write some code to compute this actual annotated code, you can see SemanticHighlightingTests.cpp for reference). #define $1[[a$1^bc]] 1 #undef $1[[abc]] #define $2[[a$2^bc]] 2 #undef $2[[abc]] #ifdef $Unknown[[abc]] #endif |
Build result: pass - 60093 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt, CMakeCache.txt
Modified tests for better error messages.
clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp | ||
---|---|---|
81 | I have tried to check against each group now. Also verifies the SymbolID. Failures will point the annotation for which the testcase failed. |
Build result: pass - 60093 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt, CMakeCache.txt
Build result: pass - 60093 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt, CMakeCache.txt
Thanks, looks mostly good, a few more nits.
clang-tools-extra/clangd/CollectMacros.h | ||
---|---|---|
90–96 | nit: abstract MacroNameTok.getIdentifierInfo()->getName() to a variable? we also used this on line 90. | |
clang-tools-extra/clangd/XRefs.cpp | ||
898 | nit: remove this change. | |
clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp | ||
2 | nit: add missing license. | |
29 | This is interesting, by "doesn't work", you mean the function could not locate the correct definitions for (the first & second abc)? | |
50 | could you add one more test case where there is a macro reference in another macro argument, like: #define M(X) X; #define $1[[abc]] 123 int s = M($1[[abc]]); | |
75 | nit: const auto & | |
81 | Thanks, this looks better now. | |
110 | nit: we need newline at EOF. |
I think the Ranges and MacrosRefs have a lot of duplications, it is wasteful to store a same range twice. We don't need the MainFilePath, the callers should know the corresponding file path for the AST. Something like llvm::DenseMap<SymbolID, std::vector<Range>> Refs; should be enough.
symbol id for macro generation is required the definition location, but only defined macros have it, we may need an extra vector to store ranges for undefined macros.
There may be more tricky cases, it would be nice to have in the test.