[cfe-dev] (Kind of) Extending the Clang AST

[Manuel Klimek]

On Wed, Oct 2, 2013 at 7:32 PM, Gabor Kozar <kozargabor at gmail.com> wrote:

> **
> Hi Jordan,
>
> The motivation was to be able to seamlessly use e.g. the hasName() matcher
> inside the containerDecl() matcher, but having to go through one additional
> matcher - wrappedNamedDecl() - is not really an issue. I also thought that
> the AST matchers would only work for AST types, but apparently I was wrong.
>
> So I've rewritten the code so that ContainerDeclWrapper is only a wrapper
> around a NamedDecl, but doesn't inherit from it.
> Thanks for your input!
>

I'd agree that using subclassing is almost certainly not the right
approach. I'm still not sure why you need a wrapper, instead of just using
some free-standing helper methods. If you have free-standing helper-methods
(isContainer, getValueType, getIteratorType), I think those would be useful
to contribute back, as those might also be very useful for example for the
clang-modernize tool.

Cheers,
/Manuel


>
> --
>  Gábor Kozár -- ShdNx
>  kozargabor at gmail.com
>
>
>
> On Wed, Oct 2, 2013, at 3:21, Jordan Rose wrote:
>
> Hi, Gábor. I don't think this is the right approach. The AST is supposed
> to be completely fixed by the time it reaches the analyzer (except for
> synthesized functions generated with BodyFarm). I get that it's convenient
> to have these accessors, but I think it's better to use them as helper
> functions or a wrapper struct after you've matched an appropriate NamedDecl
> (or actually ValueDecl, probably).
>
> AST_MATCHER_P(ValueDecl, containerDecl, ContainerDeclMatcher, innerMatcher)
> {
>     if (!isContainer(containerDecl))
>         return false;
>     return innerMatcher.matches(*containerDecl, Finder, Builder);
> }
>
> assert(isContainer(result));
> ContainerWrapper w(result);
>
> What was your motivation for using a custom subclass of NamedDecl in the
> first place?
> Jordan
>
>
> On Sep 30, 2013, at 10:18 , Gabor Kozar <kozargabor at gmail.com> wrote:
>
>
> Hi,
>
> (Note: all of the code shown below is incomplete and totally untested -
> not even sure if it compiles.)
>
> I am developing a Static Analyzer plug-in for Clang 3.3. I have created a
> class like this:
>
>
> class ContainerDecl : public NamedDecl
> {
> public:
>     static bool classof(const Decl* decl);
>
>     static bool isContainerDecl(const NamedDecl* decl);
>     static const ContainerDecl* tryCreateFrom(const NamedDecl* decl);
>
>     const NamedDecl* getWrappedDecl() const;
>     bool isValidContainer() const;
>
>     QualType getIteratorType() const;
>     QualType getItemValueType() const;
>
> protected:
>     ContainerDecl(const NamedDecl* decl);
>
> private:
>     ...
> };
>
>
> As you can see, this is basically a wrapper around a NamedDecl, and
> provides convenience methods for getting information about the container
> the NamedDecl describes.
>
> The motivation comes from writing numerous Static Analyzer checkers for
> STL containers, and these usually utilize AST matchers. I want to be able
> to write a matcher expression like this:
>
>
>
> containerDecl(hasItemValueType(hasDeclaration(namedDecl(hasName("std::auto_ptr")))))
>
>
> ... in order to detect issues with the now-depracted std::auto_ptr-s being
> put in a container, and then e.g. std::sort()-ed. (This is not a complete
> matcher expression of course.)
>
> With the above class, it is quite easy to do this:
>
>
> typedef internal::Matcher<ContainerDecl> ContainerDeclMatcher;
>
> AST_MATCHER_P(NamedDecl, containerDecl, ContainerDeclMatcher, innerMatcher)
> {
>     // TODO: lifetime management? if we don't need the containerDecl any
> longer, we should delete it
>     const ContainerDecl* containerDecl =
> ContainerDecl::tryCreateFrom(&Node);
>     if(!containerDecl) return false;
>
>     return innerMatcher.matches(*containerDecl, Finder, Builder);
> }
>
> AST_MATCHER_P(ContainerDecl, hasItemValueType, TypeMatcher, innerMatcher)
> {
>     return innerMatcher.matches(Node.getItemValueType(), Finder, Builder);
> }
>
> AST_MATCHER_P(ContainerDecl, hasIteratorType, TypeMatcher, innerMatcher)
> {
>     return innerMatcher.matches(Node.getIteratorType(), Finder, Builder);
> }
>
>
> What I am not sure about, is how to handle the lifetime of a
> ContainerDecl, since this isn't strictly part of Clang's AST - it is just
> basically a wrapper around a NamedDecl. ContainerDecl objects in general
> will be short-lived, compared to e.g. the NamedDecl objects they wrap.
>
> I noticed that there is ASTContext::Allocate and Deallocate, but I am not
> sure whether I should use it. Thoughts on this?
>
> I am also not entirely happy about having to inherit from NamedDecl, but
> otherwise - as far as I know - I cannot implement the above matchers. Is
> this even the correct approach?
>
> The other problem is making ContainerDecl play well with llvm::dyn_cast
> and the like. Right now I have the following implementation for
> ContainerDecl::classof:
>
>
> static bool ContainerDecl::classof(const Decl* decl)
> {
>     return NamedDecl::classof(decl) && isContainerDecl(static_cast<const
> NamedDecl*>(decl));
> }
>
>
> Is this correct?
>
> Also, do you think this is something that could be integrated into Clang
> at some point? I'd happy to contribute the code once it is working, and my
> boss agrees to have this code open-sourced.
>
> Thanks!
>
> --
>  Gábor Kozár -- ShdNx
>  kozargabor at gmail.com
>
>   _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131005/67e23e39/attachment.html>