On Wed, May 4, 2016 at 6:58 PM, Etienne Bergeron <etien...@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
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
