[cfe-commits] [PATCH] Provide better error messages for incorrect matchers

[Manuel Klimek]

On Thu, Sep 20, 2012 at 5:49 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Thu, Sep 20, 2012 at 1:49 AM, Daniel Jasper
> <reviews at llvm-reviews.chandlerc.com> wrote:
>> Hi klimek,
>>
>> By changing the conversion operator into a conversion constructor, we can enabled based on the template parameters leading to better error messages. E.g.: stmt(decl()) will now create an error message including:
>>
>> note: candidate function not viable: no known conversion from 'clang::ast_matchers::internal::BindableMatcher<clang::Decl>' to 'const clang::ast_matchers::internal::Matcher<clang::Stmt>' for 1st argument,
>
> I'm curious - what was the error message before? (I wonder if Clang
> could do a better job at it so that this API choice doesn't adversely
> affect usability)

Previously the compilation would break at instantiation time where
matcher.matches(node) was called, and there wouldn't be a function
'matches' of the required type. At which point, as a user, my thinking
is: "matches? I didn't call no matches?"
Makes sense?

Cheers,
/Manuel

>
>>
>>
>> http://llvm-reviews.chandlerc.com/D44
>>
>> Files:
>>   include/clang/ASTMatchers/ASTMatchersInternal.h
>>
>> Index: include/clang/ASTMatchers/ASTMatchersInternal.h
>> ===================================================================
>> --- include/clang/ASTMatchers/ASTMatchersInternal.h
>> +++ include/clang/ASTMatchers/ASTMatchersInternal.h
>> @@ -257,21 +257,23 @@
>>    explicit Matcher(MatcherInterface<T> *Implementation)
>>        : Implementation(Implementation) {}
>>
>> +  /// \brief Implicitly converts \c Other to a Matcher<T>.
>> +  ///
>> +  /// Requires \c T to be derived from \c From.
>> +  template <typename From>
>> +  Matcher(const Matcher<From> &Other,
>> +          typename llvm::enable_if_c<
>> +            llvm::is_base_of<From, T>::value &&
>> +            !llvm::is_same<From, T>::value >::type* = 0)
>> +      : Implementation(new ImplicitCastMatcher<From>(Other)) {}
>> +
>>    /// \brief Forwards the call to the underlying MatcherInterface<T> pointer.
>>    bool matches(const T &Node,
>>                 ASTMatchFinder *Finder,
>>                 BoundNodesTreeBuilder *Builder) const {
>>      return Implementation->matches(Node, Finder, Builder);
>>    }
>>
>> -  /// \brief Implicitly converts this object to a Matcher<Derived>.
>> -  ///
>> -  /// Requires Derived to be derived from T.
>> -  template <typename Derived>
>> -  operator Matcher<Derived>() const {
>> -    return Matcher<Derived>(new ImplicitCastMatcher<Derived>(*this));
>> -  }
>> -
>>    /// \brief Returns an ID that uniquely identifies the matcher.
>>    uint64_t getID() const {
>>      /// FIXME: Document the requirements this imposes on matcher
>> @@ -289,22 +291,22 @@
>>    }
>>
>>  private:
>> -  /// \brief Allows conversion from Matcher<T> to Matcher<Derived> if Derived
>> -  /// is derived from T.
>> -  template <typename Derived>
>> -  class ImplicitCastMatcher : public MatcherInterface<Derived> {
>> +  /// \brief Allows conversion from Matcher<Base> to Matcher<T> if T
>> +  /// is derived from Base.
>> +  template <typename Base>
>> +  class ImplicitCastMatcher : public MatcherInterface<T> {
>>    public:
>> -    explicit ImplicitCastMatcher(const Matcher<T> &From)
>> +    explicit ImplicitCastMatcher(const Matcher<Base> &From)
>>          : From(From) {}
>>
>> -    virtual bool matches(const Derived &Node,
>> +    virtual bool matches(const T &Node,
>>                           ASTMatchFinder *Finder,
>>                           BoundNodesTreeBuilder *Builder) const {
>>        return From.matches(Node, Finder, Builder);
>>      }
>>
>>    private:
>> -    const Matcher<T> From;
>> +    const Matcher<Base> From;
>>    };
>>
>>    llvm::IntrusiveRefCntPtr< MatcherInterface<T> > Implementation;
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>