Matching std::set::iterator

+cfe-dev, please remember to include

I don't see anything generally wrong with your approach to match
iterators. What I would do to debug this is locally editing
ASTMatchers.h to add a "llvm::outs() << FullName;" debug output into
the matchesName matcher. That way, you can see what it is actually
trying to match.

The second question should work like:

  varDecl(hasType(recordDecl(
    hasDescendent(classTemplateSpecialization(hasAnyTemplateArgument(
      refersToType(hasName("std::auto_ptr")))))))).bind("id")

This is just as a general idea, it might not yet be correct.

Cheers,
Daniel

For the second one, I altered my snippet:

MatcherProxy StlCOAPPred::getMatcher()
{
TypeMatcher type = unless(anything());

for(const auto& e : gContainers)
type = anyOf(hasDeclaration(recordDecl(hasName(e))), type);

return id(“id”,
varDecl(allOf(hasType(recordDecl(hasDescendant(
classTemplateSpecializationDecl(hasAnyTemplateArgument(
refersToType(hasDeclaration(recordDecl(hasName(“std::auto_ptr”))))))))),
hasType(type))));
}

But it does not seems to work. It do not give me any matches.

For the first one, I futher will investigate it later today, however my bet would be that, it tries to match “std::_Rb_tree_const_iterator”.

Thanks,
Gábor

Hi!

More details on matching template specializations. After further trials I even tried this piece of code:

MatcherProxy StlCOAPPred::getMatcher()
{
TypeMatcher type = unless(anything());

for(const auto& e : gContainers)
type = anyOf(hasDeclaration(recordDecl(hasName(e))), type);

return id(“id”,
varDecl(allOf(hasType(
classTemplateSpecializationDecl(hasAnyTemplateArgument(
refersToType(hasDeclaration(hasDescendant(recordDecl(hasName(“std::auto_ptr”)))))))),
hasType(type))));
}

If I remove the hasDescendant, it works flawlessly for example for vector<auto_ptr>, however it will not fork for vector<vector<auto_ptr>>. If I add that hasDescendant it will not match anything. The same applies to the code I pasted in my mail earlier. If you have any idea what could cause this issue, please tell me.

Thanks,
Gábor

Hi!

It looks like …hasType(namedDecl(matchesName(“std::set.*iterator”))) … only tries to match ::std::_Rb_tree_const_iterator, but not std::set::iterator, so I basicaly can’t match typedefs. I think it is not possible to match on typedef-ed types right now.

There is one test case:
EXPECT_TRUE(matches(“typedef int X;”, NamedX));

However it matches a typedef declaration. However if we wan’t to match something like “X i;” later on, we can not do that, however it would be extremely useful.

Any comments on this?

Thanks,
Gábor

Hi!

More details on matching template specializations. After further trials I
even tried this piece of code:

MatcherProxy StlCOAPPred::getMatcher()
{
  TypeMatcher type = unless(anything());

  for(const auto& e : gContainers)
    type = anyOf(hasDeclaration(recordDecl(hasName(e))), type);

  return id("id",
            varDecl(allOf(hasType(

classTemplateSpecializationDecl(hasAnyTemplateArgument(

refersToType(hasDeclaration(hasDescendant(recordDecl(hasName("std::auto_ptr")))))))),
                          hasType(type))));
}

If I remove the hasDescendant, it works flawlessly for example for
vector<auto_ptr<int>>, however it will not fork for
vector<vector<auto_ptr<int>>>. If I add that hasDescendant it will not match
anything. The same applies to the code I pasted in my mail earlier. If you
have any idea what could cause this issue, please tell me.

This will not work, as hasDeclaration(hasDescendant(recordDecl(...)))
is always something that matches *inner classes* (of arbitrary depth)
of the declaration.

I think we'll need to finish off the type matching that's currently
under review (blocked by a disagreement with Chandler :wink:

If you really need how many layers of vector<> you have around your
type, you can write out a finite number by now, or, if that's not good
enough for your use case, you'll need to use a hand-rolled visitor.

Cheers,
/Manuel

Hi!

It looks like ...hasType(namedDecl(matchesName("std::set.*iterator"))) ...
only tries to match ::std::_Rb_tree_const_iterator, but not
std::set<foobar>::iterator, so I basicaly can't match typedefs. I think it
is not possible to match on typedef-ed types right now.

There is one test case:
EXPECT_TRUE(matches("typedef int X;", NamedX));

However it matches a typedef declaration. However if we wan't to match
something like "X i;" later on, we can not do that, however it would be
extremely useful.

Any comments on this?

We need to write a matcher that supports this. We already have some of
the supporting code for isDerivedFrom which looks through typedefs.
We'll basically want to have a matcher matchesNameOrTypedef (minus
finding a better name for the matcher :wink: that does what you want.

Cheers,
/Manuel

We should make that decisions together with the Type/TypeLoc matching.
I think looking through typedefs needs to be done with matchers for
types. matchesName()/hasName() on declarations should always look at
the name of the type used for the declaration and not at an alias
defined in a typedef.

Cheers,
Daniel

We should make that decisions together with the Type/TypeLoc matching.
I think looking through typedefs needs to be done with matchers for
types. matchesName()/hasName() on declarations should always look at
the name of the type used for the declaration and not at an alias
defined in a typedef.

I'm not sure I agree. It seems like the name property is really one of the
decl or typedef, not one of the type.

I'm curious about other opinions, though...

Cheers,
/Manuel

From my point of view, maybe something like matchesNameOrTypedef would be clean/easy. However I will be happy with any approach that provides an obvious interface. I think not being able to match typedefs is one of the biggest shortcomings of the matchers right now.

From my point of view, maybe something like matchesNameOrTypedef would be
clean/easy. However I will be happy with any approach that provides an
obvious interface. I think not being able to match typedefs is one of the
biggest shortcomings of the matchers right now.

Can you work around that by specifying the names it has been
typedef'ed to for now?

Yeah, I need this for a tool for my thesis (finding common stl mistakes in code), but I can just use for example “std::_Rb_tree_const_iterator”, and mention in my thesis, the current implementation only supports one certain STL implementation, and will be improved in the future.

But when some experimental patches are available, I will be happy to test them out.

From: "Gábor Horváth" <xazax.hun@gmail.com>
To: "Manuel Klimek" <klimek@google.com>
Cc: "Clang Developers" <cfe-dev@cs.uiuc.edu>
Sent: Thursday, October 11, 2012 7:39:50 AM
Subject: Re: [cfe-dev] Matching std::set::iterator

Yeah, I need this for a tool for my thesis (finding common stl
mistakes in code), but I can just use for example
"std::_Rb_tree_const_iterator", and mention in my thesis, the
current implementation only supports one certain STL implementation,
and will be improved in the future.

But when some experimental patches are available, I will be happy to
test them out.

[quasi-off-topic] I'm interested in how this turns out, not only because I think that STL warnings will be a great thing to have, but also because I'm interested in this optimization: I'd like to output TBAA metadata so that the backend understands that pointers into distinct std::vector objects (and maybe other containers?) don't alias. It seems that implementing this will require the same kind of technology as what you're doing.

-Hal

@Hal

I’m using the tooling library, which is primarily for stand-alone tools, however I think it is also possible to use the matchers in the frontend, however I don’t know, if it is advised (maybe Daniel or Manuel can tell this). If it isn’t advised, it will be better to use a recursive AST visitor for this reason.

@Hal

I'm using the tooling library, which is primarily for stand-alone tools,
however I think it is also possible to use the matchers in the frontend,
however I don't know, if it is advised (maybe Daniel or Manuel can tell
this). If it isn't advised, it will be better to use a recursive AST
visitor for this reason.

It's not yet adviced - we're working on that, but we want to have Doug &
co. fully on board. One feature that's missing for that is being able to
run matchers on arbitrary parts of the AST (not only the full AST).

Hi!

I just tried out the new typedefType matcher. However there is one issue I encountered, maybe it is not intended to be used this way?

This matcher matches: callExpr(allOf(callee(functionDecl(hasName(“std::find”))),
hasArgument(0, hasType(type()))))

However this matcher do not match: callExpr(allOf(callee(functionDecl(hasName(“std::find”))),
hasArgument(0, hasType(typedefType()))))

Do you have any idea what might be the problem? It compiles both ways.

Thanks,
Gábor

To be correct, the code that I’m using for test, looks like this one:

std::set::iterator it2 = s.begin();
find(it2,s.end(),i);

To be correct, the code that I'm using for test, looks like this one:

    std::set<int>::iterator it2 = s.begin();
    find(it2,s.end(),i);

Looking at the AST:
    (CXXConstructExpr 0x489b058 <col:13> 'struct
std::_Rb_tree_const_iterator<int>':'struct
std::_Rb_tree_const_iterator<int>''void (const struct
std::_Rb_tree_const_iterator<int> &) throw()'
      (ImplicitCastExpr 0x489b040 <col:13> 'const struct
std::_Rb_tree_const_iterator<int>' lvalue <NoOp>
        (DeclRefExpr 0x489aa10 <col:13> 'std::set<int>::iterator':'struct
std::_Rb_tree_const_iterator<int>' lvalue Var 0x48974d0 'it'
'std::set<int>::iterator':'struct std::_Rb_tree_const_iterator<int>')))

I'd guess that after the ImplicitCastExpr the type is not the typedef'ed
type any more...

Cheers,
/Manuel

Thanks!

I’m now able to catch codes like this one: find(s.begin(),s.end(),i);
via a matcher like this:
callExpr(allOf(callee(functionDecl(hasName(“std::find”))),
hasArgument(0, hasDescendant(expr(hasType(typedefType(hasDecl(matchesName(“std::(multiset|set).*::iterator”))))))))).bind(“id”)

Hi!

I think there is still no way to get the typedef type of a declaration like:
std::set::iterator it;

It is a ClassTemplateSpecializationDecl, and there is no way to get its typedefType. A matchesTypedefName or a similar matcher could solve the problem.

There is one more minor thing in the API, and I wonder if it is intended.

There is:
hasDeclaration matcher for constructExpr, callExpr and QualTypes, and there is hasDecl for TypedefTypes. Maybe it would be more consistent to call both hasDeclaration?

Thanks,
Gábor

Hi!

I think there is still no way to get the typedef type of a declaration
like:
std::set<int>::iterator it;

It is a ClassTemplateSpecializationDecl, and there is no way to get its
typedefType. A matchesTypedefName or a similar matcher could solve the
problem.

There is one more minor thing in the API, and I wonder if it is intended.

There is:
hasDeclaration matcher for constructExpr, callExpr and QualTypes, and
there is hasDecl for TypedefTypes. Maybe it would be more consistent to
call both hasDeclaration?

Ah, we probably want to call all of them hasDecl... strictly speaking
hasDecl is a function of TypedefType, while the others are more complex,
but I agree that it's just confusing.

Cheers,
/Manuel