Page MenuHomePhabricator

[AST] Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl
ClosedPublic

Authored by daiyousei-qz on May 31 2022, 11:35 PM.

Details

Summary

The original discussion starts at this issue: https://github.com/clangd/clangd/issues/1132
where clangd's semantic highlighting is missing for symbols of a template specialization definition. It turns out the visitor didn't traverse the base classes of Class/Var##TemplateSpecializationDecl, i.e. CXXRecordDecl/VarDecl. This patch adds them back as what is done in DEF_TRAVERSE_TMPL_PART_SPEC_DECL.

I'm not sure who should be included as reviewers. Please edit as needed.

Diff Detail

Unit TestsFailed

TimeTest
10 msx64 debian > Clang-Unit.Tooling/_/ToolingTests/RecursiveASTVisitor::NestedNameSpecifiersForTemplateSpecializationsAreVisited
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/unittests/Tooling/./ToolingTests --gtest_filter=RecursiveASTVisitor.NestedNameSpecifiersForTemplateSpecializationsAreVisited

Event Timeline

daiyousei-qz created this revision.May 31 2022, 11:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 11:35 PM
daiyousei-qz requested review of this revision.May 31 2022, 11:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 11:35 PM

Thanks for the patch!

Could you add a semantic highlighting test case to SemanticHighlighting.GetsCorrectTokens as well please? The test cases there are pieces of code annotated with the type of highlighting kind each token should get.

clang/include/clang/AST/RecursiveASTVisitor.h
2037

We should move this call to TraverseNestedNameSpecifierLoc into the else branch, since Traverse...Helper already does that:

  • TraverseCXXRecordHelper does it via TraverseRecordHelper here
  • TraverseVarHelper does it via TraverseDeclaratorHelper here

added unit test
moved redundant visit

Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 4:47 PM
Herald added a subscriber: arphaman. · View Herald Transcript

It seems you have uploaded just the changes relative to the previous patch. If you made a second commit, you need to squash it into the first and resubmit.

Otherwise the test looks good, thanks!

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
811 ↗(On Diff #433590)

nit: // Explicit template specialization to be a bit more specific

daiyousei-qz marked an inline comment as done.

squashed commits.
updated comments.

rsmith added a subscriber: rsmith.Jun 3 2022, 4:32 PM
rsmith added inline comments.
clang/include/clang/AST/RecursiveASTVisitor.h
2041

This should be done automatically by the WalkUpFrom* implementation, not here. Do you know why that's not happening?

rsmith added inline comments.Jun 3 2022, 4:50 PM
clang/include/clang/AST/RecursiveASTVisitor.h
2041

Oh, never mind, this is doing the "traverse" part not the "visit" part. Ignore that comment!

nridge added a subscriber: sammccall.

Thanks, the patch looks good to me, but someone else should probably take a look and approve.

Adding @sammccall who has looked at RecursiveASTVisitor recently in D120498.

rsmith accepted this revision.Jun 5 2022, 8:38 PM
This revision is now accepted and ready to land.Jun 5 2022, 8:38 PM
daiyousei-qz retitled this revision from Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl to [AST] Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl.Jun 5 2022, 10:03 PM

Thanks Richard for the review!

@daiyousei-qz would you like me to commit the patch for you? (If so, I need a name + email for attribution, there doesn't seem to be one in the revision metadata.)

Yes, please do so. I'm using the web interface since the arc tool isn't very friendly to Windows user :(

Name: Qingyuan Zheng
Email: qyzheng2@outlook.com

This revision was automatically updated to reflect the committed changes.