[PATCH] D19871: Add an AST matcher for CastExpr kind

[Aaron Ballman via cfe-commits]

On Wed, May 4, 2016 at 6:58 PM, Etienne Bergeron <etienneb at google.com> wrote:
> etienneb added inline comments.
>
> ================
> Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:102
> @@ +101,3 @@
> +  static clang::CastKind getCastKind(llvm::StringRef AttrKind) {
> +    return llvm::StringSwitch<clang::CastKind>(AttrKind)
> +      .Case("CK_Dependent", CK_Dependent)
> ----------------
> aaron.ballman wrote:
>> This might be an awful idea, but let's explore it.
>>
>> What if we moved the CastKind enumerator names into a .def (or .inc) file and use macros to generate the enumeration as well as this monster switch statement? We do this in other places where it makes sense to do so, such as in Expr.h:
>> ```
>>   enum AtomicOp {
>> #define BUILTIN(ID, TYPE, ATTRS)
>> #define ATOMIC_BUILTIN(ID, TYPE, ATTRS) AO ## ID,
>> #include "clang/Basic/Builtins.def"
>>     // Avoid trailing comma
>>     BI_First = 0
>>   };
>> ```
> Does the dynamic matching is used somewhere else than clang-query?

They are available for use elsewhere, but I'm not certain if it *is*
used (it's a library component).

> I wonder the impact of refactoring to support them if it's barely used.

clang-query is not barely used. It's frequently my first choice for
testing out matchers, and having functionality that cannot be used
there is a source of annoyance.

> It can't be worse than before as it wasn't supported at all (the matcher didn't exists).

I disagree. Divergence between the dynamic matchers and what we can
use from C++ should be a big, red flag.

> I believe there is a larger cleanup to do to support correctly dynamic matcher like "equals".
> And, this case is one among others.
>
> I'm not a fan of this huge switch that may just get out-of-sync with the original enum.

That is why I'm recommending using the .inc/.def approach. Then
there's no chance of getting out of sync.

> I'm still in favor of adding this matcher to the unsupported list until we push the more complicated fix.
> [which may fall in my plate anyway]

I would rather not have the matcher than add it to the unsupported
list. I view that list the same way I view the "diagnostics with no
flags" list: it should never grow, only shrink. Another way to think
of it: this matcher is only used by C++ and only for a clang-tidy
check currently. There's no harm in leaving it exposed only to that
check until we are able to expose it properly as a more general AST
matcher for public consumption, or until we get a lot more checks that
require the functionality.

~Aaron

> Any toughs?
>
>
> http://reviews.llvm.org/D19871
>
>
>