This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Store xref for Macros in ParsedAST.
ClosedPublic

Authored by usaxena95 on Nov 8 2019, 7:01 AM.

Details

Reviewers
hokein
Summary

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.

Diff Detail

Event Timeline

usaxena95 created this revision.Nov 8 2019, 7:01 AM

Please ignore the changes from patch https://reviews.llvm.org/D69937
Will fix this.

usaxena95 updated this revision to Diff 228456.Nov 8 2019, 7:25 AM

Removing changes from different patch.

hokein added inline comments.Nov 11 2019, 3:01 AM
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.

usaxena95 updated this revision to Diff 228679.Nov 11 2019, 5:41 AM
usaxena95 marked an inline comment as done.

Rebase

usaxena95 added inline comments.Nov 11 2019, 5:44 AM
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

usaxena95 updated this revision to Diff 228869.Nov 12 2019, 4:40 AM
usaxena95 marked 2 inline comments as done.

Added tests for CollectMacros.h
Addressed comments.

usaxena95 updated this revision to Diff 228870.Nov 12 2019, 4:47 AM

Fixed typos.

usaxena95 marked an inline comment as done.Nov 12 2019, 4:48 AM
usaxena95 added inline comments.
clang-tools-extra/clangd/CollectMacros.h
29
  • using llvm::DenseMap<SymbolID, std::set<Range>> Refs; as de-duplication is easier.
  • Iterating on all the ranges is a pain. Let me know if you want to provide a different method to return all the ranges (sad part: lot of copying).
  • I had multiple definitions of a macro in the test case, but I have shifted to its separate test case.

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

hokein added inline comments.Nov 14 2019, 4:29 AM
clang-tools-extra/clangd/CollectMacros.h
29

as de-duplication is easier.

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]]
usaxena95 marked 7 inline comments as done.Nov 14 2019, 7:04 AM
usaxena95 added inline comments.
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).
So a single macro per test case wouldn't be able to test this.

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.

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
usaxena95 updated this revision to Diff 229492.Nov 15 2019, 3:06 AM
usaxena95 marked 2 inline comments as done.

Uploading latest patch

Build result: pass - 60093 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt, CMakeCache.txt

usaxena95 updated this revision to Diff 229529.Nov 15 2019, 6:20 AM
usaxena95 marked 2 inline comments as done.

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.
Recreating the annotated code from the raw code would become more complex as it would involve maintaining a mapping from SymbolId->Annotation. Works quite nicely for SemanticHighlighting as we have the whole Token stream. Creating the replacements does not sound trivial to me.

Build result: pass - 60093 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt, CMakeCache.txt

usaxena95 updated this revision to Diff 229535.Nov 15 2019, 6:46 AM

Minor change: Resued variable.

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.

usaxena95 updated this revision to Diff 229836.Nov 18 2019, 6:57 AM
usaxena95 marked 8 inline comments as done.

Addressed comments.

usaxena95 marked an inline comment as done.Nov 18 2019, 6:58 AM
usaxena95 added inline comments.
clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp
29

It is not able to locate the macro for second definition.
Causes assertion failure at line 93: assert(Macro);

75

ranges returns a copy and not a reference. I would expect copy elision here and & would be unnecessary.

hokein accepted this revision.Nov 18 2019, 7:14 AM

lgtm, thanks!

This revision is now accepted and ready to land.Nov 18 2019, 7:14 AM
usaxena95 closed this revision.Dec 4 2019, 7:31 PM
usaxena95 marked an inline comment as done.