alexfh added inline comments.
================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:33-36
+ anyOf(hasAncestor(
+ functionTemplateDecl(HasMatchingDependentDescendant)),
+ hasAncestor(
+ classTemplateDecl(HasMatchingDependentDescendant))))
----------------
astrelni wrote:
> alexfh wrote:
> > The hasAncestor and hasDescendant matchers frequently have non-trivial
> > performance implications. Especially the latter, especially when called on
> > a large number of nodes with a large number of transitive children (this
> > obviously depends on the code being analyzed). I don't know how likely is
> > this to cause problems here (the matcher seems to only be used when the
> > check is about to issue a diagnostic), but it's always good to be aware of
> > the possible issues.
> >
> > Frequently there's a more efficient (and easier to understand) alternative.
> > For example, instead of trying to evaluate certain conditions while
> > traversing the code (which may require a large number of lookups into
> > relatively distant parts of the AST), it's sometimes more efficient to
> > split the analysis into two or more stages. During the AST traversal (i.e.
> > when AST matchers run) the check could collect some raw information about
> > all potentially problematic places in the code and then (for example, in
> > `onEndOfTranslationUnit`) analyze the collected information together in a
> > second stage. This works best if there's a way to arrange the data gathered
> > on the first pass such that the second pass can efficiently look up
> > necessary information.
> Thanks, gave it a try, what do you think of this?
From a cursory look this should be less likely to be sloooow. In any case,
makes sense to profile the check on a number of large files (at least against
other checks, using clang-tidy's -enable-check-profile option).
================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:43
+ unless(hasTemplateArgument(0, refersToType(builtinType()))),
+ anyOf(hasName("operator*="), hasName("operator/="))))),
+ this);
----------------
hasAnyName, please. It's more efficient. A few more instances below.
https://reviews.llvm.org/D53830
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
