Page MenuHomePhabricator

[clangd] Add dlog()s for SelectionTree, enabling -debug-only=SelectionTree.cpp
ClosedPublic

Authored by sammccall on Mon, Jul 22, 5:08 AM.

Details

Summary

SelectionTree is a RecursiveASTVisitor which processes getSourceRange() for
every node. This is a lot of surface area with the AST, as getSourceRange()
is specialized for *many* node types.
And the resulting SelectionTree depends on the source ranges of many
visited nodes, and the order of traversal.

Put together, this means we really need a traversal log to debug when we
get an unexpected SelectionTree. I've built this ad-hoc a few times, now
it's time to check it in.

Example output:

D[14:07:44.184] Computing selection for </usr/local/google/home/sammccall/test.cc:1:7, col:8>
D[14:07:44.184]  push: VarDecl const auto x = 42
D[14:07:44.184]   claimRange: </usr/local/google/home/sammccall/test.cc:1:12, col:13>
D[14:07:44.184]   push: NestedNameSpecifierLoc (empty NestedNameSpecifierLoc)
D[14:07:44.184]   pop: NestedNameSpecifierLoc (empty NestedNameSpecifierLoc)
D[14:07:44.184]   push: QualifiedTypeLoc const auto
D[14:07:44.184]   pop: QualifiedTypeLoc const auto
D[14:07:44.184]    claimRange: </usr/local/google/home/sammccall/test.cc:1:7, col:11>
D[14:07:44.184]    hit selection: </usr/local/google/home/sammccall/test.cc:1:7, col:8>
D[14:07:44.184]   skip: IntegerLiteral 42
D[14:07:44.184]    skipped range = </usr/local/google/home/sammccall/test.cc:1:16>
D[14:07:44.184]  pop: VarDecl const auto x = 42
D[14:07:44.184]   claimRange: </usr/local/google/home/sammccall/test.cc:1:1, col:18>
D[14:07:44.184]  skip: VarDecl int y = 43
D[14:07:44.184]   skipped range = </usr/local/google/home/sammccall/test.cc:2:1, col:9>
D[14:07:44.184] Built selection tree
TranslationUnitDecl
  VarDecl const auto x = 42
     .QualifiedTypeLoc const auto

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Mon, Jul 22, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jul 22, 5:08 AM
hokein accepted this revision.Mon, Jul 22, 6:43 AM

looks good.

This revision is now accepted and ready to land.Mon, Jul 22, 6:43 AM
This revision was automatically updated to reflect the committed changes.
bjope added a subscriber: bjope.Sat, Jul 27, 1:39 AM

Since this patch we get

../../clang-tools-extra/clangd/Selection.cpp:80:13: error: unused function 'printNodeToString' [-Werror,-Wunused-function]
std::string printNodeToString(const DynTypedNode &N, const PrintingPolicy &PP) {
            ^
../../clang-tools-extra/clangd/Selection.cpp:351:25: error: private field 'PrintPolicy' is not used [-Werror,-Wunused-private-field]
  const PrintingPolicy &PrintPolicy;
                        ^
2 errors generated.

When building with -DNDEBUG and -Werror.

I guess we can sprinkle some ifndef NDEBUG in this file, but I'm not sure how these dlog statements normally are handled in clang-tools-extra. Should for example LLVM_ENABLE_DUMP also be used for clang-tools-extra?

Since this patch we get

../../clang-tools-extra/clangd/Selection.cpp:80:13: error: unused function 'printNodeToString' [-Werror,-Wunused-function]
std::string printNodeToString(const DynTypedNode &N, const PrintingPolicy &PP) {
            ^
../../clang-tools-extra/clangd/Selection.cpp:351:25: error: private field 'PrintPolicy' is not used [-Werror,-Wunused-private-field]
  const PrintingPolicy &PrintPolicy;
                        ^
2 errors generated.

When building with -DNDEBUG and -Werror.

I guess we can sprinkle some ifndef NDEBUG in this file, but I'm not sure how these dlog statements normally are handled in clang-tools-extra. Should for example LLVM_ENABLE_DUMP also be used for clang-tools-extra?

Well, I made a fix here: https://reviews.llvm.org/rL367178