bloomberg / clangmetatool

A framework for reusing code in Clang tools
https://bloomberg.github.io/clangmetatool/
Apache License 2.0
121 stars 25 forks source link

Fix reference count bug for fully qualified types #65

Closed nsmith0 closed 3 years ago

nsmith0 commented 3 years ago

Issue number of the reported bug or feature request: #

None

Describe your changes

clang's AST matchers are matching twice on a fully qualified type reference like: Bar::Zab::Foo.

Once for: Bar::Zab::Foo And once for: Foo

So for respective file UIDs, IncludeGraphData's usage_reference_count is set to 2 for a line like this: Bar::Zab::Foo foo; when it should only be set to 1.

My fix involves tracking the end locations of RecordTypes. The postulate is that two RecordTypes that end at the same location are in fact the same RecordType. If we find two or more RecordTypes with the same end location, count them as 1. Can anyone think of an example that invalidates this assumption, and is there a more clangy way of fixing this, perhaps with matchers?

Testing performed

Added two new unit tests, 038 and 039, which test correct fully qualified reference counts.

dbeer1 commented 3 years ago

Bar::Zab::Foo is an ElaboratedTypeLoc, you can get from it to the bare TypeLoc Foo using getNamedTypeLoc(). Both of them will resolve to the same declaration in most cases, so deduplicating them using their position like you are doing is generally okay.

However, there is one case where they will not overlap: if Foo is declared via the using keyword, the declaration of bare TypeLoc will point to the wrong place, in particular it will point to the original definition of the class referenced in the using statement.

As an example:

// A.h
namespace A {
class X {};
}

// B.h
namespace B {
using Y = A::X;
}

// test.cpp
B::Y var

In this case, the declaration of B::Y will point to A::X in A.h, which is wrong. The solution is to resolve the "qualifier" of the ElaboratedTypeLoc, aka B, into a namespace and then look up the bare TypeLoc, aka Y, in the namespace. This will give you the using statement that you can then add a reference to.

is this worth doing here?